mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 03:16:43 +00:00
6f8105b01dd9dcfeff4d9307bf0f40fbb0984cfa
576 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
6f8105b01d |
refactor(attachments): consolidate icon helpers onto an SVG set (TASK-2417)
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 |
||
|
xarmian
|
b90e7edaeb | docs(attachments): record the lock-held pool I/O hazard at the call site (BUG-2409) | ||
|
xarmian
|
e12feb46cb |
fix(copy): authorize attachment references in cross-workspace copy (TASK-2408)
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. |
||
|
xarmian
|
2318a17e49 |
fix(attachments): classify derived rows after authorization on delete
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. |
||
|
xarmian
|
ba848af85f |
fix(attachments): check restriction before the role gate on orphan delete
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.
|
||
|
xarmian
|
1da96106e8 |
refactor(attachments): centralize parent resolution, close orphan-read and delete-denial gaps
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.
|
||
|
xarmian
|
9ad718178d |
fix(store): workspace-scope the item-grant lookup (TASK-2403)
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 |
||
|
xarmian
|
90eb871da3 |
fix(attachments): skip derivation for an archived parent (TASK-2404)
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 |
||
|
xarmian
|
380b75e12c |
fix(attachments): gate transform on item visibility (TASK-2402)
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
|
||
|
xarmian
|
6e2b972fb0 |
fix(attachments): gate blob reads on item visibility (TASK-2401)
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
|
||
|
xarmian
|
27b71fe4f6 |
fix(attachments): resolve item_id across both upload channels (TASK-2400)
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 |
||
|
xarmian
|
eae42b843e |
fix(store): scope attachment list JOINs by workspace (TASK-2399)
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. |
||
|
xarmian
|
e115bb255e |
feat(web): delete attachments from the item strip (TASK-2384)
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
|
||
|
xarmian
|
d9d96b85c9 | refactor(store): delete two unused item-workspace-move accessors (TASK-2374) | ||
|
xarmian
|
3bbd326857 | test(store): make the copy concurrency and attachment assertions bite (TASK-2372) | ||
|
xarmian
|
c6ebe5a3e3 |
refactor(store): unify the collection column list and scan (TASK-2368)
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. |
||
|
xarmian
|
98c638fc86 | refactor(server): extract resolveAuthorizedCopy shared by preflight and copy (TASK-2370) | ||
|
xarmian
|
c783d36a13 |
fix(store): make migration 077 constraint-equivalent to 055 per final review
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
|
cfc83e8c57 |
fix(server): report partial and legacy relationships in the copy dry-run (TASK-2369)
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 |
||
|
xarmian
|
66fa464699 |
fix(store): distinguish SQLite lock timeout from a real deadlock per final review
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 |
||
|
xarmian
|
2b9da9412b |
fix(store): classify unique violations as expected copy rejections per final review
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 |
||
|
xarmian
|
f15ba86db0 |
docs(server): correct the cross-workspace authz re-check contract per final review
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 |
||
|
xarmian
|
1e48a7a1dd |
feat(cli): add pad item copy for cross-workspace copy and move (TASK-2366)
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.
|
||
|
xarmian
|
f8ff5742e5 |
feat(server): add cross-workspace copy endpoint with post-commit fanout (TASK-2365)
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
01d640978c |
feat(server): add cross-workspace copy dry-run preflight endpoint (TASK-2364)
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
0fad869a28 |
feat(store): add CopyItemAcrossWorkspaces atomic orchestration (TASK-2363)
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
|
||
|
xarmian
|
60dd3e1a37 |
feat(store): add attachment resolution planner for cross-workspace copy (TASK-2354)
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 |
||
|
xarmian
|
dbede59edf |
refactor(store): extract tx-taking item creation helper (TASK-2362)
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 |
||
|
xarmian
|
1eb1c9eda6 |
feat(server): expose ACL-gated moved-to pointer on item GET (TASK-2359)
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 |
||
|
xarmian
|
5804c80146 | feat(server): add cross-workspace authorization helper (TASK-2358) | ||
|
xarmian
|
bf14c1168a |
feat(store): add item_workspace_moves provenance table (TASK-2356)
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 |
||
|
xarmian
|
faf9b3734a |
feat(web): default new collections to Board — schema-aware (IDEA-2274, IDEA-2287) (#1015)
* feat(web): default new collections to Board view (IDEA-2274) Board becomes the baseline default view for new collections; existing collections keep their stored default_view (no migration). - Frontend fallback (settingsDefaults, collection-page defaultMode, shareView coerce, initial viewMode) -> board - Create/Edit collection modals default -> board - Backend template seeds (defaults.go, templates*.go) list -> board for ideas/plans/docs/hiring/interviewing collections (tasks was already board) - CLI `pad collection create` and MCP mapCollectionCreate defaults -> board - Curated create-modal presets with deliberate list curation (Meeting Notes, Decisions, OKRs) intentionally left as list - Pin the three list-keyboard-nav pane E2E tests to ?view=list Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): board default reaches public share page + ItemDetail fallback (Codex round 1) Codex review found the public share route (s/[token]) derives its owner default view via a separate `?? 'list'` fallback that bypassed the coerceSettings change, so settings-less/legacy collections rendered List on public share pages. Align it (and the pre-init selectedBase) to board. Also align ItemDetail's inline CollectionSettings fallback (default_view is unused there, but keep it consistent with settingsDefaults). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(collections): group Contacts board by relationship, not status (Codex round 2) Contacts has no `status` field, so defaulting it to Board grouped by the default `status` rendered every card in a single Uncategorized lane. Set BoardGroupBy=relationship so the board shows real lanes. All other board-defaulted seed collections have a status field or an explicit board_group_by (verified: Companies/Conventions/Playbooks/Docs have status). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): always serialize ?view= so a List URL survives a board default (Codex round 3) buildCollectionUrlParams treated List as the implicit URL view and omitted it. With Board now a possible collection default (IDEA-2274), a List selection on a board-default collection produced a URL that, when copied or opened without the sender's localStorage, resolved back to Board. Always serialize the view mode; add a covering unit test. Verified the pane E2E suite (URL-equality assertions) stays green. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
51cd6e84e4 |
fix(collab): op-id durable fence for restore-rollback vs applier-ack race (BUG-2276 residual 2)
Closes the restore-rollback vs applier-ack clobber race with a durable operation-id correlation instead of a timing heuristic. The client brackets its setContent with an applier_apply_start{request_id} control frame; the server decides whether the external write persisted by reading the per-conn op-log high-water UNDER the same appendMu that sets the restore freeze (finalize-at-freeze — no drain, so a blocked write can't stall the restore; no timing window). Edges handled: unanchored conns are never elected; gate admission spans registration; legacy (pre-bracket) clients negotiate capability and an unconfirmable legacy round-trip returns a retryable 409 applier_ambiguous (fail-safe, never a clobber); the applier callback is synchronous-by-type so nothing can split the bracket. Normal acks stay on a lock-free, latency-identical fast path.
Confirming Codex (high effort): redesigned from a timing grace after review; 3 rounds on the op-id design (2 P1 -> 3 P1+P2 -> CLEAN/converging). E2E + Go(PostgreSQL) green; go test -race clean 8x. Go/Web CI red only on the pre-existing dependency advisories (BUG-2278).
https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
|
||
|
xarmian
|
e601f2b368 |
fix(collab): reconcile Postgres commit-ack-loss on version restore instead of treating it as rollback (BUG-2276 residual 1)
On Postgres, a version-restore commit that durably lands but whose ack is lost surfaced as an error and wrongly resumed peers on a stale Y.Doc. ForceRefreshRoom now runs a Postgres-only reconcile after a commit error: two durable signals (content == restored version AND last_restore_seq advanced past a lock-captured baseline) must agree → LANDED (publish fences + reseed, return the restored item + SSE); both false → rolled back (unfreeze); disagree/read-error → UNCERTAIN (invalidate in-memory fences so durable state governs, then plain-close sockets so peers reconnect + re-evaluate). SQLite path unchanged. Confirming Codex (high effort): 3 rounds — false-404, frozen-forever, archive-nil, stale-baseline, stale-in-memory-fence-clobber all closed; real Postgres end-to-end ack-loss + SSE test. make test-pg green. Residual 2 (applier-ack rollback race) follows separately. Go CI red only on the pre-existing govulncheck advisory (BUG-2278). https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
37ec77d110 |
fix(store): monotonic tie-breaker for same-second item version ordering (BUG-2270)
Adds a per-item monotonic `version_seq` column (dual migrations: SQLite 076 / Postgres 054, backfilled via ROW_NUMBER) so version-history RECONSTRUCTION resolves same-second versions deterministically instead of by the random-UUID PK. Reconstruction paths (shouldCreateItemVersion, ListItemVersions/Resolved, export) order by version_seq; the timeline keyset path (ListItemVersionsBeforeTime) keeps its id-consistent cursor. Confirming Codex (high effort): found + fixed one keyset-pagination P2 (order/cursor key mismatch). make test-pg green (migration verified against Postgres). Go CI job red only on the pre-existing govulncheck advisory tracked in BUG-2278. https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
40f88052cd |
fix(collab): version restore via prune+reseed (BUG-2264) (#990)
Version restore didn't reconcile the live Y.Doc: peers kept editing a Y.Doc built on pre-restore ops, and their next collab-snapshot flush clobbered the restored items.content. Reworked restore to prune+reseed — the restored content becomes canonical and every peer converges on it (unflushed edits are discarded, which is exactly restore semantics), replacing the earlier applier/epoch/watermark routing. handleRestoreItemVersion drives RoomManager.ForceRefreshRoom under the per-item lock. Hardened across Codex xhigh review rounds: - Atomicity: pre-prune MAX(op-log), the items.content write, the "Restored from…" version, the op-log wipe, AND both durable restore boundaries all run in ONE store transaction. A failed commit rolls back all of it — no divergent state, no fail-open boundary. - Unambiguous commit signal: UpdateItem reads the updated row WITHIN the tx (getItemTx) before commit, so a read failure can't make a committed update look failed and the returned seq is this restore's. - Restore freeze: conns are paused via a dedicated rc.frozen flag (NOT canWrite) so the auth-revalidation loop can't thaw the freeze mid-restore or promote a viewer; pickApplier + the applier-ack handler reject frozen conns so a concurrent external PATCH can't falsely succeed. - Stale-flush boundary: pre-prune MAX+1 fences in-flight snapshot cursors under the same item lock. - force_refresh fan-out deadlock: per-conn timer-close so a wedged writeLoop can't hang the fan-out + item lock. - Stale-SEED clobber: the client announces the item.seq it seeded from (?content_seq=) on every (re)connect; Join force_refreshes any seed that predates the last restore. Residual #1 (restart-durability) CLOSED durably, for BOTH stale vectors — the in-memory fences didn't survive a restart, so a surviving cursor-0 pre-restore browser tab wasn't fenced on reconnect. Two nullable per-item columns (migration 075 SQLite / pg 053), both stamped in the restore's own tx (atomic with the content write + op-log prune): * items.last_restore_seq — the content generation. Join's stale-seed fence reads it (via store.ItemLastRestoreSeq) when the in-memory fast-path misses (after a restart); if that read errors, Join fails CLOSED via a RETRYABLE plain close (not a force_refresh, which would discard the Y.Doc and spin an unbounded refresh loop) so the client reconnects with backoff, Y.Doc intact. * items.restore_boundary_op_id — the op-log-id boundary. The collab-snapshot flush gate reads it (via store.ItemRestoreBoundaryOpID) when the in-memory RestoreBoundary misses (after a restart), failing closed (409) on a read error, so a surviving tab's stale HTTP flush is fenced too. No SCHEMA_VERSION bump — durable columns are not a Y.Doc node-spec change. Deferred to BUG-2276: (a) a Postgres commit whose ack is lost is treated as rolled-back (needs commit-outcome reconciliation; SQLite unaffected); (b) a restore rollback racing an in-flight external-applier ack can drop the ack and retry/fall back (needs the applier flow serialised under itemLock at a 30s-stall cost). NOTE(BUG-2270): ForceVersion can mint same-second version rows; the item_versions ordering tie-breaker is tracked separately. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
1dbe04399a |
fix(store): optimistic concurrency + sibling broadcast for collection settings (BUG-2265) (#989)
* fix(store): optimistic concurrency for collection settings writes (BUG-2265) Collection-level settings (e.g. quick_actions) were written by reconstructing the whole settings JSON from a caller's local Collection snapshot, and UpdateCollection replaced the column with no concurrency check. Two ItemDetails in the same collection (full-page pane host master + pane) hold independent snapshots and clobbered each other. Mirror the item optimistic-concurrency pattern (IDEA-1480): add CollectionUpdate.ExpectedUpdatedAt; when set, UpdateCollection re-reads updated_at atomically under the workspace write lock (SQLite BEGIN IMMEDIATE / Postgres advisory xact lock) and returns CollectionUpdateConflictError on a mismatch. Empty token keeps the legacy last-write-wins path unchanged for CLI/MCP/API callers. No DB migration — reuses collections.updated_at. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * feat(server): collection.updated broadcast + 409 conflict mapping (BUG-2265) - handleUpdateCollection boundary-validates expected_updated_at (400 on a malformed token) and maps store.CollectionUpdateConflictError to the shared update_conflict envelope (HTTP 409) — byte-identical wire shape to the item path, via the extracted writeUpdateConflictEnvelope helper. - Add the collection_updated EventBus type and publish it after a successful update so sibling ItemDetails / collection pages refresh their independent Collection snapshot proactively, shrinking the 409 window. Routed by Collection (slug) through the existing SSE visibility filter. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): 409-aware collection settings writes + sibling refresh (BUG-2265) - CollectionUpdate carries expected_updated_at; add isUpdateConflictError. - QuickActionsMenu sends the token and, on a 409, refetches the collection, re-appends the new action onto the FRESH settings, and retries once — no silent loss, no user-visible error. - EditCollectionModal captures the token at open-time (edge-gated seed so a concurrent broadcast can't wipe in-progress edits) and shows a non-destructive "changed elsewhere, reload" message on 409 rather than auto-merging a full-form edit. - Subscribe to collection_updated over SSE: ItemDetail and the collection page refresh their own Collection snapshot (gen/slug-fenced against the persistent pane host's no-remount switch), so siblings converge before the next save. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): harden collection optimistic concurrency + web fetch ordering (BUG-2265, Codex round 1) Address Codex review findings: - [P1] same-second clobber: now() is one-second precision, so two guarded writes in the same second kept an identical token. The accepted write now advances updated_at strictly past the token (only when now() hasn't already moved on), making a stale-token replay deterministically conflict. Add a same-second regression test. - [P1] tokenless-writer race on Postgres: the advisory lock only serialized writers that also took it. Replace it with a `FOR UPDATE` row lock on the in-tx re-read (Postgres) — SQLite's BEGIN IMMEDIATE already serializes every writer — so a concurrent tokenless UpdateCollection can't slip between the re-read and the UPDATE. - [P2] rename broadcast: only publish collection_updated when the slug is unchanged. A rename's old-slug event would make siblings refetch a dead slug (404) and a new-slug event can't reach old-slug visibility snapshots; renames are handled by the existing navigation path. - [P2] out-of-order refreshes: ItemDetail and the collection page now use a dedicated monotonic refresh counter so two rapid collection_updated fetches can't resolve out of order and clobber newer state (loadSeq/loadGeneration only bump on route/item loads). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): make collection updated_at strictly monotonic for ALL writes (BUG-2265, Codex round 2) The previous same-second advance ran only on guarded updates, so a tokenless UpdateCollection could write the current second over a forced expected+1s, regressing the concurrency token and letting a stale guarded client clobber newer data (Codex P1). Route every collection update through one small transaction that re-reads updated_at (FOR UPDATE on Postgres; SQLite BEGIN IMMEDIATE covers it) and derives the new timestamp atomically: strictly advance past the row's current value when now() hasn't already moved on. This makes updated_at a reliable concurrency token for guarded AND tokenless writers. Add a tokenless-monotonic regression test. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: close remaining collection-concurrency gaps (BUG-2265, Codex round 3) - [P1] Board column reordering (handleGroupReorder) rebuilt the full schema from a stale local snapshot and wrote it with no token — a lost-update path identical to the bug being fixed. Now sends expected_updated_at and, on 409, refetches, re-applies the reorder onto the fresh schema, and retries once. - [P2] The workspace settings page seeded EditCollectionModal from a page-load-time collections list, so a change that predated editing produced a false 409. It now refreshes the list on collection_updated (seq-guarded). - [P2] collection.updated is now delivered to item-grant-only SSE subscribers for collections they can see — it's itemless but leak-free (only the slug), so guests' ItemDetail schema/settings snapshots converge too. Filter test extended. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): switch-safety + conflict-merge fixes for collection writes (BUG-2265, Codex round 4) - Board column reorder now ABORTS on a 409 (with a "reorder again" toast) instead of replaying a stale option order onto the fresh field, which would silently drop a concurrent option add/remove/rename. Reordering is cosmetic; never worth clobbering a real schema edit. Also captures ws/slug/base before the await and fences the write against a route switch. - QuickActionsMenu captures workspace + collection identity BEFORE the first await, so a mid-save navigation can't make the 409 refetch/retry target the wrong collection (no guaranteed remount). - Settings-page SSE refresh captures the workspace and drops the result if the workspace changed while fetching, so a slow refresh for workspace A can't overwrite workspace B's freshly loaded collection list. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): sub-second collection updated_at token, no future drift (BUG-2265, confirming pass #6) The same-second monotonic advance manufactured whole-second FUTURE updated_at values; sustained >1 write/sec on one collection drifted arbitrarily ahead of wall-clock. collections.updated_at is TEXT on both dialects and never compared lexically (only via time.Equal + display), so switch the update write to sub-second nowNano(): same-second collisions become near-impossible, so the token advances naturally. Keep a strict-monotonicity guard but step by a single NANOSECOND on the (now near-impossible) coarse-clock/step-back collision, so any drift is bounded to nanoseconds. Dual-dialect; covered by make test-pg. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(server): sanitize + always-broadcast collection event (BUG-2265, confirming pass #2,#3) - #2 (P1): collection_updated is delivered to item-grant guests, but the event carried ActorName/Source, leaking the owner's identity + edit source. Strip them — publishCollectionEvent now emits workspace + slug (+ new_slug) only. - #3 (P2): always broadcast (including on rename), routed by the OLD slug and carrying the NEW slug via a new Event.NewSlug field, so remote tabs on the old slug can re-target instead of silently 404ing on their next action. Tests: assert no actor/source leak on a settings update; assert a rename routes by old slug + carries new_slug. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): decisive switch-safety + rename handling for collection writes (BUG-2265, confirming pass #1,#3,#4,#5) - #1 (P1): EditCollectionModal captures the target collection id/slug/name/ws + updated_at when the form is SEEDED, and handleSave/handleArchive now operate on that captured identity (not the live props). The seed effect re-seeds when the collection IDENTITY changes (not on a same-id broadcast refresh), so a reused route can't leave A's form saving/deleting to B. - #3 (P2): on a rename event the collection route navigates to the new slug (preserving the pane query) and ItemDetail refetches by new_slug; the SSE event type carries new_slug. - #4 (P2): the reorder-conflict path refetches the collection (reseeds the token) before prompting, so a missed SSE event doesn't make every retry 409 forever. - #5 (P2): QuickActionsMenu only invokes oncollectionupdated when the live workspace/slug still match the captured identity, so a reused route can't assign an old response to the newly-navigated page. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(server): nano token round-trip, rename visibility, publish-before-migration (BUG-2265, confirming pass 2) Address the round-2 confirming-pass findings (server-only): 1. (P2) The shared update_conflict envelope formatted actual_updated_at with second precision (time.RFC3339), truncating the now sub-second collection token so the client's retry token never matched — a permanent 409 loop. Format with time.RFC3339Nano. Item tokens are zero-nanosecond, so RFC3339Nano emits no fractional part — the item 409 wire shape is byte-identical and the item path still compares via time.Equal. Added a test that the returned token round-trips as a usable retry token. 2. (P2) Rename events are routed by the OLD slug, but a subscriber that revalidated after the rename only has the NEW slug in visibleSlugSet, so the visibility check dropped the event before the new_slug branch. Accept a rename when EITHER the old slug or the (authorized) NewSlug is visible; downstream item-grant gating uses whichever slug is visible. Filter test extended. 3. (P2) The collection_updated event was published only after field migrations succeeded, but UpdateCollection already committed (updated_at advanced). On a migration failure clients got a 500 and no refresh, leaving siblings with stale tokens that 409 blindly. Publish on the commit (before the migration), so siblings always resync regardless of migration outcome. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: atomic collection update+migration; modal same-id rename retarget (BUG-2265, confirming pass 3) Address the round-3 confirming-pass findings; defer cross-tab rename RE-NAVIGATION to BUG-2272 (placeholder) per coordinator. 1. (P1) Migration atomicity. UpdateCollection committed the schema + concurrency token BEFORE MigrateItemFieldValues ran, so a migration failure returned 500 with the row already changed → the retry was guaranteed to 409 and item values were left inconsistent with the committed schema. Made the two ATOMIC: extracted applyFieldMigrationsTx and run it inside UpdateCollection's own transaction (after taking the workspace seq lock), so a migration failure rolls back the schema AND the token — nothing changes, the retry works. The handler now passes migrations through instead of running them separately, and publishes the event only after the fully-atomic commit. store/tx work → make test-pg run green. 2. (P2) EditCollectionModal same-id rename. The round-1 identity capture ignores same-id prop refreshes (to preserve edits), but a concurrent RENAME changes the slug (not the id), so handleSave/handleArchive PATCHed a dead slug → 404 before the token could 409. On a same-id prop change whose slug changed, the seed effect now retargets the endpoint slug + re-captures the token WITHOUT reseeding the form (in-progress edits preserved). Deferred (BUG-2272, TODO comments added, already broken on main — no regression): - ItemDetail full-page item URL/collSlug not retargeted after a remote rename. - Collection route chained-rename events during SSE replay landing on a dead intermediate slug. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): keep seeded token on same-id rename retarget (BUG-2265) On the EditCollectionModal same-id rename branch, retarget the endpoint (slug/name/ws) only — drop the token re-capture. Re-capturing let a later handleSave succeed against the renamed collection and apply the modal's stale pre-rename full form, silently REVERTING the concurrent rename (the exact stale-snapshot clobber BUG-2265 prevents). Keeping the seeded token means a concurrent rename correctly yields a 409 → the non-destructive "collection changed, reload" message. Slug-retarget without token-recapture gives both: no 404 (right URL) and no clobber (409 fires). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: lock-order deadlock + unified collection-snapshot fences (BUG-2265, confirming pass 4) 1. (P1) DEADLOCK regression. UpdateCollection's atomic migration path took the collection-row FOR UPDATE lock and THEN the workspace seq lock, but item creation takes them in the reverse order (workspace advisory lock first, then the collection-row FK lock on INSERT) — a concurrent item-create + schema-migration ABBA-deadlocks on Postgres. Fix: acquire the workspace seq lock BEFORE the collection-row FOR UPDATE (matching item-create's order). Every store path that locks both now takes them workspace-seq → collection-row (tryCreateItem, UpdateItem, MigrateItemFieldValues, UpdateCollection). Added a concurrency regression test (item-create racing schema-migration); make test-pg green. 2+3. (P2) Cross-generation fence gap. The SSE collection refresh and route/item loads used SEPARATE counters, so a stale in-flight load could complete after a fresh SSE refresh and revert the collection + its concurrency token. Unified to a SINGLE monotonic collection-snapshot generation in BOTH the collection route and ItemDetail — every collection-snapshot write (loadCollection/loadData, the SSE refresh, reorder, and the quick-action/edit-modal callbacks) bumps it on start and gates its assignment on "still latest". ItemDetail's load keeps a switch-escape so a stale refresh for the OLD collection can't block loading a NEW one. Settings page unified the same way over its collections-list writes. 4. (P2) Settings page fed a stale editingCollection to the edit modal after a remote rename (its prop never changed → the same-id-rename retarget never fired → 404). The unified refresh now re-points editingCollection at the refreshed object for the same id, so the modal's retarget fires. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: emit item-changes signal on field migration; QuickActions retry-by-id (BUG-2265, confirming pass 5) 1. (P1) A collection update that runs a field migration mutates item `fields` JSON and advances item `seq`, but only collection_updated was published — open item views refreshed collection METADATA and returned without reconciling the migrated items, so clients kept stale field JSON under the new schema and a later full-fields item update could UNDO the migration (a clobber). UpdateCollection now returns the migrated-item count; when > 0 the handler ALSO emits the existing bulk item-mutation signal (items_bulk_updated, Op=migrate) so open views reconcile via /items-changes. Fires only when the migration touched >= 1 item — a pure settings/quick-actions update emits nothing extra. No store SQL/locking change (Go signature + count plumbing only); make test / make test-pg both green. 2. (P2) QuickActionsMenu's 409 retry GET-by-slug 404s if the competing update renamed the collection. Resolve the fresh collection by STABLE id (list + find by id) before re-appending + retrying, mirroring EditCollectionModal's identity approach; the result-propagation guard is now id-based too so a rename doesn't spuriously drop it. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: uniform sweep of item-grant delivery, rename routing, and 409/404 retries (BUG-2265, confirming pass 6) One pattern-sweep instead of per-site patches. Audited every event this PR publishes and every client retry path, applying three patterns uniformly: PATTERN A (item-grant SSE reconcile) + B (old-slug rename routing): instead of a SEPARATE items_bulk_updated migration event (which carries op/count for items an item-grant subscriber can't see and isn't rename-routed), FOLD a SANITIZED `items_changed` bool onto collection_updated — already item-grant-delivered (round 3) and already old-slug-routed with new_slug (round 2). On it the client triggers a /items-changes deltaSync (server-filtered to the caller's grants) and ItemDetail refetches its open item, so item-grant EDITORS reconcile migrated field JSON — closing the clobber where a stale full-fields update would UNDO the migration. Leak surface: "a collection you can see items in changed [+ renamed + had item changes]" — a bool, no per-item data. Removed the round-5 items_bulk_updated publish. The pre-existing items_bulk_updated (archive/move) is untouched and correctly stays suppressed for item-grant users. PATTERN C (409 AND 404 in retries): a competing RENAME can 404 a slug-targeted write before it can 409, bypassing recovery. Added isNotFoundError / isConflictOrNotFound helpers; every write/retry path now treats BOTH: QuickActions save resolves-by-id and retries on either; board reorder reseeds-by-id and aborts on either; EditCollectionModal save shows the reload prompt and archive resolves-by-id and retries on either. Tests: server asserts collection_updated sets items_changed on migration (not on settings-only) and stays sanitized; the SSE-filter test asserts the migration variant reaches item-grant subscribers for a visible collection; web unit tests assert 404/409 classification and a real component-driven not_found -> resolve- by-id -> retry in QuickActionsMenu. make test / make test-pg / npm run test all green. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: stable collection-ID identity for collection events + request-based items_changed (BUG-2265, confirming pass 7) 1. (P1) Collection events were identified only by MUTABLE, reusable slugs, and events replay — so a stale rename event's old slug, once re-owned by a DIFFERENT collection, could pass a slug-based match and misroute a client (navigate away / load the wrong schema) or leak the new slug. Fix at the ROOT: carry the STABLE CollectionID on collection_updated (Event.CollectionID) and match by ID everywhere: - Server visibility: sseEventVisibleFor matches collection_updated on a new visibleCollIDSet (built from the same VisibleCollectionIDs), not the slug — so an event for a collection the subscriber can't see by ID is dropped even if its (reused) slug is in visibleSlugSet. Filter test proves the slug-reuse drop. - Clients: ItemDetail and the collection route match `event.collection_id === <their collection>.id`, not slug. Slug(s)/new_slug stay only for the rename-navigation URL. Settings refreshes its whole list (already id-safe). 2. (P1) items_changed was keyed off the affected-ROW count, delivered to item-grant subscribers → a subscriber whose own items were unaffected could infer that HIDDEN items matched the migrated value. Now keyed off whether a field MIGRATION WAS REQUESTED (len(input.Migrations) > 0), independent of row count — leaks nothing about hidden item values. Reverted round-5's UpdateCollection count-return (no longer needed). Test: a migration matching ZERO items still sets items_changed. Deferred with markers: - NOTE(BUG-2273) at ItemDetail's reconcile-skip AND updateField: the web editor's full-fields field write lacks item-level OCC (never adopted IDEA-1480/v0.14), so the migration reconcile is best-effort. - TODO(BUG-2272) at the reorder 404 reseed: it refreshes `collection` but not the route `collSlug` (renavigation, deferred). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: archive OCC (no destructive wrong-target) + settings load fence (BUG-2265, confirming pass 8) 1. (P1) EditCollectionModal handleArchive resolved the target by stable id but the server DELETE re-resolves by the MUTABLE slug — a rename that re-owned that slug before the delete landed would archive the WRONG collection. Close the TOCTOU with an expected_updated_at OCC on the delete, mirroring the update OCC: DeleteCollection re-reads updated_at under a lock (FOR UPDATE on Postgres) and 409s on mismatch; the handler validates the token + maps the 409; the client sends it as a query param; handleArchive passes the seeded token (and the fresh token on the resolve-by-id retry). A reused slug or a concurrently-changed target now yields a clean 409 → the reload message, never a wrong-collection archive. Server test: stale token 409s (and the collection survives); current token 204s; malformed 400s; no token 204s. 2. (P2) settings load(): the generation was bumped AFTER awaiting setCurrent, so a slow load for workspace A could resume after B's load and clobber B's name/context/collections/members. Capture a dedicated loadGen at load() ENTRY (before any await) and fence EVERY continuation on it; the collections write additionally respects collectionsGen so it can't revert a fresher SSE refresh. Using a dedicated loadGen (not the SSE-shared collectionsGen) means an SSE collections-refresh mid-load doesn't drop the name/members writes. Deferred: TODO(BUG-2272) at the collection route's rename-navigation site — the global collectionStore (sidebar/pickers) isn't refreshed and the workspace layout ignores collection_updated, so the sidebar keeps the dead slug. Layout- level renavigation, deferred. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): dedicated item-snapshot fence + id-based rename comparisons in ItemDetail (BUG-2265, confirming pass 9) One comprehensive ItemDetail async-snapshot fence sweep so this file's item/ collection fencing is uniform and ID-based. 1. (P1) The migration item-refetch and loadData shared loadGeneration, so the refetch could apply migrated fields and then a stale loadData response overwrite them (a later full-fields edit then undoes the migration). Added a DEDICATED itemGen (separate from loadGeneration and collectionGen), bumped at the start of BOTH loadData's item load AND the migration refetch, and gated BOTH `item = ` writes on "still latest itemGen" — neither can stale-overwrite the other. Swept the other PASSIVE item snapshot-refreshes onto itemGen too (SSE item_updated/archived/restored, onSync deleted/incremental/full, the collab refresh) so they're ordered against each other and the migration/load. 2. (P2) A settings update that follows a rename before the rename fetch completes requested the OLD slug and bumped collectionGen, cancelling the valid rename fetch. Fetch slug is now `event.new_slug || event.collection || slug`. 3. (P2) The loadData collection fence-escapes compared the stale load's SLUG vs the freshly-renamed snapshot's slug (they differ on a rename → escape let the stale result overwrite). They now compare stable collection IDs; the SSE refresh's post-fetch identity check is id-based too. Audit (site -> generation -> id?): every PASSIVE snapshot-refresh (loadData item+collection, migration refetch, SSE x3, onSync x3, collab) bumps the correct dedicated gen (item->itemGen, collection->collectionGen) and compares identity by id. The DELIBERATE user/action writes (title/field/tag/assignee/role/content/ link/version/restore saves) keep loadGeneration + item-id switch-safety; their item-snapshot concurrency vs the migration refetch is the deferred item-OCC gap (BUG-2273, best-effort) — reordering them last-started-wins is orthogonal to that. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
bcef802335 |
fix(security): gate collab WebSocket writes on editor role (TASK-265) (#938)
* fix(security): gate collab WebSocket writes on editor role (TASK-265)
The collab WebSocket (GET /api/v1/collab/{itemID}) is mounted outside
the /{slug} subrouter, so RequireWorkspaceAccess never runs on it.
authorizeCollabAccess gated admission on membership + item visibility
but NOT edit role, so a plain workspace VIEWER was admitted and could
WRITE: every inbound Yjs sync frame persisted to item_yjs_updates
(room.go) and got canonicalized into items.content when a co-present
editor's authorized flush ran. The REST write path blocks viewers via
requireEditPermission; this closes the equivalent gap on the collab
relay.
Fix — non-editors become READ-ONLY participants (not hard-rejected, so
live view + presence stay intact):
- authorizeCollabAccess now returns a collabAccess{canWrite} alongside
the admission decision. canWrite is computed once via
store.ResolveUserPermission (the same predicate requireEditPermission
falls back to): owner/editor membership grants write; a viewer/guest
gets write only through a collection/item edit grant.
- RoomManager.Join takes a canWrite flag stored per-connection as an
atomic.Bool. room.go's readLoop drops a read-only conn's inbound sync
frames (not persisted via AppendYjsUpdate, not rebroadcast); awareness
(presence) frames still relay so the viewer's cursor stays visible,
and outbound broadcasts from editors still reach the viewer.
- The handler's periodic revalidation pushes mid-session write-permission
changes via a new RoomManager.SetConnWritable, so an editor demoted to
viewer becomes read-only without a reconnect (complements the existing
CloseConn-on-revocation path).
No SCHEMA_VERSION / DefaultSchemaVersion bump: this is an authorization
/ behavioral change, not a ProseMirror/Y.Doc node-spec change, so the
op-log must not be pruned.
Tests: TestCollabViewerIsReadOnly (viewer admitted 101, receives an
editor's broadcast, but its own sync frame is neither persisted nor
broadcast while the editor's is) and TestAuthorizeCollabAccessCanWrite
(viewer→canWrite=false, editor→canWrite=true). Verified the E2E test
fails with the gate removed.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): close 4 collab read-only gaps from orchestrator review (TASK-265)
Independent Codex pass on the collab editor-role gate found four gaps:
[P1] Read-only conns were still eligible designated APPLIERS. A viewer
(or an editor demoted mid-session) could be elected to apply an
external content edit; its resulting sync frames were dropped by the
new gate, yet its applier_ack was accepted → ApplyExternalContent
reported success → the PATCH handler skipped its direct-write fallback
→ the external edit was silently lost. Fix: pickApplier now skips
non-writers, and handleControlMessage ignores applier_ack from a conn
whose canWrite is false (belt-and-suspenders so the fallback fires).
[P2] Demotion TOCTOU. readLoop read canWrite=true, then could block on
appendMu and persist AFTER SetConnWritable(false) returned. Fix: the
canWrite check now runs INSIDE the appendMu critical section, and
SetConnWritable stores the flag under the same appendMu — so a frame
racing a demotion is either fully persisted before the flip or dropped.
[P2] Revalidation could run before the conn was registered. The first
jittered tick could fire while Join was still setting up; SetConnWritable
would no-op against the unregistered conn and Join then installed the
stale canWrite=true until a later tick. Fix: Join takes an onRegistered
callback invoked right after addConn; the handler gates the reval loop
on it so the first SetConnWritable always finds the conn.
[P2] canWrite didn't mirror REST for editors/owners. It was computed
purely from ResolveUserPermission, which resolves item/collection
GRANTS before membership role — so an editor/owner holding an
incidental `view` grant was wrongly made read-only. Fix: mirror
requireEditPermission exactly — editor/owner MEMBER short-circuits to
canWrite=true BEFORE grant resolution; viewers/guests still fall back
to ResolveUserPermission so grants can override an insufficient role.
Tests: TestApplyExternalContentSkipsReadOnlyApplier (verified failing
without the pickApplier gate), TestHandleControlMessageIgnoresAckFromReadOnlyConn,
TestCollabDemotionMakesConnReadOnly (mid-session demotion → read-only
without reconnect), and two new TestAuthorizeCollabAccessCanWrite cases
(editor+incidental view grant → true; viewer+edit grant → true).
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): safe no-applier direct write for read-only-only collab rooms (TASK-265)
Codex round 2 found a P1 introduced by excluding viewers from applier
election: in a room whose only peers are read-only, an external content
update (PATCH) hits ErrNoApplierAvailable, then PruneAndApply refused to
prune because live conns existed (len(r.conns) > 0). After the retry
budget the PATCH handler fell through to an UNLOCKED, UN-PRUNED direct
write — items.content was updated but the stale op-log survived, so a
fresh editor replaying it (or a viewer promoted to editor flushing its
stale in-memory Y.Doc) would silently overwrite the external update.
Fix: PruneAndApply now blocks only on a live WRITER peer — a read-only
peer can never persist, so it doesn't force the unsafe fallback. After
the prune + write succeeds it evicts the read-only peers
(Room.closeReadOnlyConns: WriteControl close frame + Close, concurrency-
safe with writeLoop) so their now-stale Y.Doc can't linger; they
reconnect and lazy-seed from the fresh items.content (their old resume
cursor is below the pruned op-log's MIN → force_refresh). Mixed rooms
(an editor present) are unaffected — the editor is still elected applier
and PruneAndApply is never reached.
Tests: TestPruneAndApplyEvictsReadOnlyRoom (read-only-only room prunes +
evicts, applyFn runs) and TestPruneAndApplyBlockedByLiveWriter (a live
writer still yields ErrRoomActiveDuringPrune).
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): fence PruneAndApply read-only eviction under appendMu (TASK-265)
Codex round 3 P1: PruneAndApply classified writers, ran applyFn (prune +
write), and evicted read-only conns WITHOUT holding room.appendMu. A
concurrent viewer→editor revalidation could set canWrite=true after the
writer check, append a stale frame during the prune/write, and — now a
writer — evade closeReadOnlyConns, racing the prune and leaving a live
stale Y.Doc that overwrites the external update.
Fix: PruneAndApply now holds room.appendMu across the ENTIRE sequence
(writer classification + applyFn + eviction). appendMu is the same lock
readLoop takes across its canWrite-check+persist and SetConnWritable
takes when flipping canWrite, so a promotion can no longer interleave
with the classification/prune. Lock order is itemLock → appendMu →
room.mu; no path takes room.mu → appendMu, so no inversion.
Also closes the residual "frame already read, blocked on appendMu, then
promoted after release" window: roomConn gains a terminal `evicted`
atomic flag set by closeReadOnlyConns (under appendMu) and checked in
readLoop's persist gate alongside canWrite, so an evicted read-only
conn's in-flight frame is dropped even if a racing revalidation promotes
it in the same instant.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* refactor(collab): descope read-only eviction; keep writer-aware prune guard (TASK-265)
Per orchestrator scope decision, remove the read-only EVICTION machinery
added during review (over-engineering for TASK-265's security goal):
- Room.closeReadOnlyConns and its call in PruneAndApply.
- roomConn.evicted flag and its check in readLoop's persist gate.
- appendMu held across PruneAndApply + the force-close socket I/O.
PruneAndApply reverts to no appendMu / no socket I/O, keeping only the
LOAD-BEARING writer-aware guard: it blocks (ErrRoomActiveDuringPrune)
only on a live WRITER peer, not any conn. An all-viewer room's external
edit therefore still prunes + direct-writes safely (op-log pruned, so a
fresh editor lazy-seeds from the new items.content) instead of erroring.
The residual — a connected read-only peer keeps a possibly-stale Y.Doc
until reconnect/refresh, and a viewer promoted to editor before re-sync
could push stale content — is a low-severity lost-update edge (a
promoted viewer is a legitimate editor), consistent with the pre-existing
direct-write contract. Documented on PruneAndApply and tracked in
BUG-2103 (proposed fix: proactive re-seed/refresh of remaining read-only
peers).
Kept unchanged: the authorizeCollabAccess canWrite editor/owner role
short-circuit, dropping read-only inbound sync frames under appendMu +
the SetConnWritable demotion fence + registration ordering, and the
pickApplier / applier_ack read-only exclusions.
Tests: replace TestPruneAndApplyEvictsReadOnlyRoom with
TestPruneAndApplyAllowsReadOnlyRoom (read-only-only room -> applyFn
runs); keep TestPruneAndApplyBlockedByLiveWriter.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): enforce token write-scope + fence prune promotion on collab (TASK-265)
Two logic gaps from the orchestrator's final pass:
[P1] canWrite ignored BEARER-TOKEN SCOPE. The collab upgrade is a GET,
so a read-scoped PAT/OAuth token passes TokenAuth's method-keyed
tokenScopeAllows check, then rode the user's editor role (or the legacy
workspace-token grant) to canWrite=true and could persist Yjs mutations
over the socket — a read-only-principal-writes bypass via token scope
instead of role. REST DOES enforce write-scope (TokenAuth →
tokenScopeAllows blocks read-scoped tokens from PATCH/POST/DELETE); the
collab GET simply slips the method gate. Fix mirrors REST: TokenAuth now
stashes the token scopes (WithTokenScopes, as MCPBearerAuth already
does), and authorizeCollabAccess re-applies the write-capability half —
canWrite is downgraded to read-only when the caller's token scope
doesn't permit writes (http.MethodPost representative verb). Applied to
both the legacy workspace-token path and the member/grant path.
Non-token principals (cookie / CLI session, fresh install) carry empty
scopes → unrestricted → unaffected. Test: an editor with a read /
pad:read token gets canWrite=false; with write / * gets canWrite=true.
[P2] PruneAndApply's writer-scan was not serialized with SetConnWritable,
so a viewer promoted during applyFn could append a stale frame while the
op-log is pruned + content written (persist/prune ordering race, distinct
from BUG-2103's async residual). Fix: hold room.appendMu across the
writer-scan AND applyFn. Safe now that eviction/socket-I/O is gone —
applyFn is a pure store op (PruneYjsUpdatesBefore + UpdateItemWithParentLink,
the only caller) that never re-enters itemLock / appendMu / room.mu, so no
inversion or re-entrant deadlock. Lock order: itemLock → appendMu → room.mu.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): honor token write-scope in collab fresh-install branch (TASK-265)
The zero-user (pre-bootstrap) branch of authorizeCollabAccess returned
canWrite=true unconditionally. A legacy workspace token still carries a
scope on a fresh instance, so a read-scoped token could persist Yjs
mutations over the collab GET upgrade — inconsistent with REST, whose
method gate blocks a read token's mutation. Route the branch through
collabTokenWriteScopeAllowed, which returns true for the anonymous
(no-token) setup caller (empty scopes = unrestricted) and false for a
read-scoped token. Adds TestAuthorizeCollabAccessFreshInstallTokenScope.
Found by the orchestrator's independent Codex pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
|
||
|
xarmian
|
2aa4f141e3 |
fix(security): require workspace membership on SSE subscriptions (TASK-264) (#937)
The SSE events stream (`GET /api/v1/events?workspace=`) resolves the workspace via resolveWorkspace. The slug form is membership-scoped (GetWorkspacesBySlugForUser → nil → 404), but the UUID form resolves through the GLOBAL, unscoped GetWorkspaceByID. So an authenticated non-member passing another workspace's UUID reached SubscribeIfAllowed with a fully-resolved workspace and no explicit membership check. Events are still fail-closed filtered by computeSSEVisibility and the stream is torn down within ~60s by the revalidation tick, so it is not a data leak — but the open connection is a connection-slot DoS and a workspace-existence oracle (200+connected vs 404), and it diverged from the explicit membership gate enforced by RequireWorkspaceAccess and the collab sibling authorizeCollabAccess. Add an entry membership/grant gate in handleSSE for regular (non-admin) user-context callers: admit only direct workspace members or guest-grant holders; otherwise return 404 (matching the slug path — a 403 would itself be an existence oracle). Admin-via-cookie keeps its platform-wide bypass; admin-via-bearer and the legacy-token / fresh-install paths are gated by the pre-existing branches above and left untouched. Regression test asserts a non-member is rejected with 404 at the SSE entry for BOTH the slug and UUID forms (and consumes no connection slot), while a legitimate member still connects (200 + connected). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
xarmian
|
8e16501e8a |
fix(security): scope webhook + token mutations by workspace to close cross-workspace IDOR (TASK-266) (#936)
* fix(security): scope webhook + token mutations by workspace to close cross-workspace IDOR (TASK-266)
handleDeleteWebhook, handleTestWebhook, and handleDeleteToken looked their
object up by ID with no workspace-ownership predicate. requireMinRole("owner")
only proves the caller owns the URL's workspace — not that the {webhookID} /
{tokenID} belongs to it — so any owner of any workspace could delete or test
another workspace's webhook, or revoke its API token, given the object ID
(cross-workspace IDOR / integrity + DoS + existence oracle).
Fix, matching the existing pre-fetch-and-compare idiom used by
handleDeleteWorkspaceAttachment / views / comments / links:
- webhooks: pre-fetch via GetWebhook and 404 unless hook.WorkspaceID matches
the URL workspace (delete + test paths).
- tokens: new store.DeleteAPITokenScoped(id, workspaceID) doing
DELETE ... WHERE id = ? AND workspace_id = ?, mirroring DeleteUserAPIToken.
The unscoped DeleteAPIToken (its only caller) is removed.
Adds TestWebhookTokenCrossWorkspaceIDOR: an owner of workspace B cannot
delete/test A's webhook or revoke A's token via B's URL (404, objects survive),
while the legitimate owner still can within their own workspace. Verified
red-before/green-after.
Part of PLAN-259 (pre-open-source security audit). Closes the webhook+token
half of TASK-266; views/comments/links were already scoped.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): atomic workspace-scoped webhook delete per Codex review (round 1)
handleDeleteWebhook pre-fetched via GetWebhook, which decrypts the HMAC
secret. A rotated/missing encryption key or corrupted ciphertext would make
the delete return 500, leaving a broken webhook undeletable. Replace the
pre-fetch-and-compare with an atomic store.DeleteWebhookScoped(id, workspaceID)
(DELETE ... WHERE id = ? AND workspace_id = ?) — same idiom as the token fix,
no decrypt on the delete path. The unscoped DeleteWebhook (its only caller) is
removed. handleTestWebhook keeps GetWebhook since it needs the decrypted hook
to dispatch.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): scope test-webhook lookup before decrypt per Codex review (round 2)
handleTestWebhook fetched via GetWebhook (SELECT + decrypt by ID) and only then
compared workspace, so an undecryptable foreign webhook returned 500 rather than
404 — a residual cross-workspace existence oracle. Add store.GetWebhookScoped(id,
workspaceID) which applies the workspace_id predicate in SQL before decrypting;
a foreign/missing ID returns (nil,nil) → 404 without touching the ciphertext.
Strengthen the regression test's victim webhook with a non-empty secret so the
scoped-before-decrypt path is exercised.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
|
||
|
xarmian
|
9f6c1d8f47 |
fix(security): scope MCP workspace-global reads by OAuth consent allow-list (BUG-2102) (#935)
The OAuth token consent allow-list (TokenAllowedWorkspaces) was enforced only
by RequireWorkspaceAccess, which fires solely for /{slug} path-param routes.
Every MCP-reachable read that is workspace-global or takes the workspace as a
query/body param bypassed the gate, so a token consented to workspace A could
reach data in other co-membership workspaces. Investigation found five
bypasses; this closes all of them:
- pad_search (HIGH): fan-out (no workspace) searched ALL memberships; naming a
workspace returned its item titles + content. Now the fan-out is restricted
to the allow-list and a named non-consented workspace returns empty (no
existence leak).
- pad_workspace.list (the original BUG-2102): filtered by the allow-list.
- pad_workspace.deleted: filtered by the allow-list.
- pad_workspace.audit-log: platform-wide admin surface; denied for
consent-scoped tokens.
- pad_workspace.restore: gated by the allow-list (404 for out-of-consent slugs).
All gates are no-ops for nil/wildcard allow-lists, so PAT auth, web sessions,
and local stdio are unchanged.
The allow-set semantics move into internal/server as the canonical
TokenAllowedWorkspaceSet(ctx) (promoted from internal/mcp's buildAllowSet);
the two mcp call sites (error-hint lister, workspaces resource) and its unit
tests migrate with it, so server handlers and MCP filters share one
implementation instead of drifting per-surface (the pattern that caused this
bug: TASK-977 and TASK-2101 each point-fixed one surface).
Tests: per-handler regression tests carrying the WithTokenAllowedWorkspaces
context MCPBearerAuth produces; each asserts the consent layer (not membership)
drives exclusion, with nil/wildcard baselines guarding against over-blocking.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
|
||
|
xarmian
|
8cdfb8e287 |
feat(mcp): remote /mcp resource parity — wire read-only resources onto the cloud transport (TASK-2101) (#934)
* feat(mcp): wire read-only resources onto remote /mcp transport (TASK-2101)
The cloud /mcp Streamable HTTP transport registered zero resources
("resources_wired: false") — the stdio ExecResourceFetcher shells out to
the pad binary with one user's ~/.pad credentials, unusable in the shared
multi-OAuth-user process, so resources (incl. PR #930's attachment image
resource) were deferred.
Add HTTPResourceFetcher: the in-process equivalent that dispatches each
resource read through the same pad-cloud handler chain, reusing
HTTPHandlerDispatcher's user resolution + buildAuthedRequest (token-scope
check, verified-email gate, consent Apply). It reproduces each CLI
--format json shape (item list -> cli.ToItemSummaries; workspace list ->
{slug,name,updated_at}; attachment show -> HEAD-header synth; dashboard/
collections/bootstrap/item show -> endpoint body). Because it satisfies
ResourceFetcher + BinaryResourceFetcher, RegisterResources wires the full
read-only set onto the remote transport with the SAME handlers stdio uses
(formatItemAsMarkdown, attachment bounds/sniff/base64) — zero duplication.
Attachment bytes flow through cappedResponseWriter (wrapping the existing
cappedWriter) preserving PR #933's 1 MiB download bound in the shared
process. mcp-go propagates the HTTP request context (WithCurrentUser) into
resource handlers, so auth/scope/consent parity with tool calls holds.
- item list resource matches CLI `--all` (lifts non_terminal only; does
NOT set include_archived — soft-deleted items stay hidden).
- Shared synthesizeAttachmentMetadata between the pad_attachment tool and
the resource fetcher so the HEAD-derived shape can't drift.
No ToolSurfaceVersion bump — resources aren't part of the tool catalog
contract (PR #930 precedent).
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(mcp): scope workspaces resource by OAuth consent allow-list per Codex review (round 1)
The pad://workspaces resource shelled GET /api/v1/workspaces, whose handler
returns every membership without consulting the OAuth token's allowed_workspaces
consent list (unlike per-workspace routes). On the remote transport a token
consented only for workspace alpha could enumerate names/slugs of unconsented
workspaces. Filter with the same rule the error-hint lister uses (buildAllowSet):
nil/wildcard allow-list -> no filter (PAT + local stdio unaffected); a specific
allow-list -> intersect with memberships.
Note: the pad_workspace list TOOL hits the same endpoint and has the same
unfiltered behavior — a pre-existing, broader concern to address at the
handler/tool level separately.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
|
||
|
xarmian
|
475a70b57a |
fix(mcp): harden attachment image resource label + download bound (#933)
Follow-up to #930: label the blob from downloaded bytes (TOCTOU fix), bound FetchBytes buffering at the 1 MiB limit, and fix stale 'deferred to TASK-2076' docs. Adversarial-review + Codex findings; Codex CLEAN. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra |
||
|
King Star
|
c07f6d4b7e |
Add bounded MCP image attachment resource (#930)
Read-only MCP resource pad://workspace/{ws}/attachments/{id} returning a bounded base64 image via the existing thumb-md variant pipeline (image-only, 1 MiB pre-base64 cap, local-stdio surface). Closes #906. Implements TASK-2076/TASK-2077.
Author: @jstar0 (first-time contributor).
|
||
|
xarmian
|
c72fe5a663 |
feat(items): add unparented filtering contract (#926)
* feat(items): add unparented filtering contract * fix(items): preserve unparented projection state * fix(views): preserve reserved filter on reset * fix(items): resync projection scope changes * fix(items): address PR 926 review findings - localIndex: fetch snapshot before clearing store/cache in resyncProjectionScope (no data-loss window on fetch failure) - items: degrade to committed item when post-parent-link readback fails instead of 500 - items: treat unparented=<non-true> as a field filter so a schema field named unparented still filters - persistence: delete dead persistCursor - mark validateUnparentedListRequest canonical; cross-reference the 3 early-feedback copies * fix(items): resync race + purge safety per Codex review (round 1) - resyncProjectionScope: merge-reconcile instead of blunt clear so a higher-seq upsert/delta racing the snapshot fetch is preserved (not erased) and the cursor never regresses below it - recheck generation after persistWipe so a sign-out/403 purge during the wipe can't resurrect purged rows via persistDelta - snapshot rows authoritatively replace local copies (drop is_unparented on downgrade); mergeRow's projection-preservation is bypassed for resync * fix(items): sanitize projection bit on preserved racing rows per Codex review (round 2) When a projection resync lands a restricted snapshot, strip is_unparented from any racing higher-seq row kept by the seq guards — the old scope no longer grants it. Keep the row itself (dropping it would reintroduce the racing-mutation data loss; server 403 enforces real visibility). * fix(items): transactional cache replace in resync per Codex review (round 3) Replace wipe()+persistDelta() in resyncProjectionScope with a single persistReplace() transaction (clear + write in one tx). Avoids the deleteDatabase() onblocked cross-tab hang where a pending delete stalls the following reopen+write indefinitely, wedging the resync promise. wipe() stays for the sign-out / schema-mismatch full-teardown paths. * fix(items): drop-and-replay resync reconciliation per Codex review (round 4) Rework resyncProjectionScope: drop every row absent from the authoritative snapshot (not just older-than-cursor ones) and pin the cursor to the snapshot cursor. A post-snapshot mutation the client can still see is re-fetched by the next /items-changes?since=cursor under the NEW scope, so visible rows return and old-scope-hidden rows stay gone — no old-scope row survives the resync, and nothing is permanently lost. Present-in-snapshot racing edits are still kept (is_unparented stripped under a restricted scope). * fix(items): continue delta poll after resync so replay actually fires (round 5) The drop-and-replay resync (round 4) pins the cursor to the snapshot cursor so post-snapshot mutations re-fetch under the new scope — but both poll loops broke out / returned immediately after the resync, so the replay never ran until an unrelated sync/reload. Both callers now continue the loop from the pinned cursor; resync already aligned the scope so the branch can't re-fire, and the existing 50-iteration cap bounds it. * fix(items): keep pendingResync set until replay catches up (round 6) resyncProjectionScope cleared pendingResync after installing the snapshot but before the pinned-cursor replay drained. If that replay later failed or hit the 50-page cap, pendingResync stayed false and the next bootstrap() no-opped with racing mutations still missing. Let the reconcile loop's caughtUp logic own the flag instead. * fix(items): set pendingResync when any resync begins (round 7) Round 6 removed the premature clear but only the bootstrap path pre-sets pendingResync; a page deltaSync resync ran with it false, so a failed/capped replay there wouldn't trigger a bootstrap resume. Set pendingResync=true at the start of resyncProjectionScope so any caller marks catch-up pending; the reconcile loop clears it on caughtUp. * fix(items): fence stale optimistic writes + epoch-guard resync catch-up (round 8) Adds a resync-epoch + fenced-id mechanism to close the last two race classes: - fencedIds: a resync records the ids it dropped (hidden under the new scope). upsert() refuses a fenced id, so a stale old-scope create/update response resolving after the resync can't resurrect a now-hidden row that no new-scope delta would evict (P1). An authoritative applyDelta re-add un-fences; the next resync recomputes the set (re-upgrade clears it). Self-contained in the store — no epoch threading through the optimistic callers. - scopeEpoch: bumped when a resync installs a new snapshot. Both reconcile loops capture it before each /items-changes and skip treating a response that raced a concurrent resync as caught-up, so a stale in-flight delta can't clear pendingResync without validating the pinned cursor (P2). Regression test covers fence → reject stale upsert → authoritative re-add un-fences → later edits accepted. * fix(items): bump scope epoch before resync fetch (round 9 P2) scopeEpoch advanced only after listIndex() returned, so a reconcile response racing the fetch saw the old epoch and could clear the pendingResync the resync set at start. Bump the epoch before the network await instead. |
||
|
Ronnie Li
|
e9d308a64e | fix(agent): support OpenCode install target (#923) | ||
|
xarmian
|
c8492db29f |
fix(e2e): disable rate limiting on the E2E server to stop 429 flakes (BUG-2089) (#922)
The E2E harness runs the real pad binary with the real rate limiter, and every Playwright test shares one loopback IP (127.0.0.1). The auth limiter (5 logins/min/IP, burst 5) trips as soon as a spec logs in a couple of browser clients — collab-persistence.spec.ts logs in two per test — so browserLogin fails with "in-page login failed with status 429". This was deterministic, not flaky: it failed on TASK-2058's own PR and its push to main, and on every downstream PR since. Add a test-only env knob PAD_DISABLE_RATE_LIMITS: when truthy, New() leaves Server.rateLimiters nil, which RateLimit() already treats as a pass-through (Stop() and the MCP path are already nil-safe). Wire it into the Playwright webServer.env; run-pad.mjs spawns the binary with inherited env so it reaches the pad process. Limiters stay fully active in prod/self-host — the knob is an explicit opt-in only the E2E server sets. Verified: collab-persistence.spec.ts passes locally with the fix; the existing limiter tests still pass (limiters on when the env is unset); new TestRateLimit_DisabledByEnv pins the bypass. Claude-Session: https://claude.ai/code/session_015yuBJQYfDj95cgX3DaD8SF |
||
|
xarmian
|
9938a81328 |
fix(oauth): default absent authorize scope so consent completes (BUG-2088) (#921)
An OAuth authorize request may legally omit `scope` (RFC 6749 §3.1.2; our advertised scopes_supported is only advisory). Claude Code does. When it does, ar.GetRequestedScopes() is empty, renderConsent shows zero capability-tier radios, and clicking Authorize dead-ends the /authorize/decide POST with "capability_tier must be 'read', 'write', or 'admin'". Default an absent scope to the requesting client's registered scopes (DCR seeds pad:read/pad:write), falling back to pad:read pad:write. A missing/unknown client_id is left untouched so fosite emits its normal invalid_client error. The default is set on BOTH r.Form (feeds NewAuthorizeRequest → the consent tier radios) AND r.URL.RawQuery: renderConsent builds the consent form's hidden authorize fields from r.URL.Query(), and /authorize/decide rebuilds the AuthorizeRequest from those hidden fields — so without the URL update the decide POST would reconstruct a scope-less request and reject the chosen tier one step later. Regression test drives the full GET-consent → POST-decide → code flow, extracting the scope from the rendered hidden field so it fails if the field goes missing. Claude-Session: https://claude.ai/code/session_015yuBJQYfDj95cgX3DaD8SF |
||
|
xarmian
|
fce3023bc5 |
test(server): adopt t.Parallel() in heaviest handler tests (TASK-2059) (#918)
The internal/server suite ran almost fully serially (only 1 of 113 test files used t.Parallel). The CI -race flake (BUG-1913) is structurally fixed via the copy-once storetest template DB, but the suite regrows toward the -timeout budget as it grows serially. Add t.Parallel() as the first line of every top-level Test* func in the four heaviest files — handlers_items, handlers_oauth, handlers_mcp, handlers_dashboard (159 tests). All build on isolated per-test fixtures (testServer / oauthEnabledTestServer, both backed by storetest.NewSQLite, which copies a fresh template DB into t.TempDir per call), so each test owns its DB, rate limiters, and event bus. Deliberately left serial: t.Run subtests that share the parent's server+workspace and mutate the same rows (e.g. the PatchItem subtests all PATCH one seeded item) — parallelizing those would race. The goroutine-count / timing-sensitive tests in server_test.go are untouched. go test -race ./internal/server/ stays clean (7m16s, exit 0). Claude-Session: https://claude.ai/code/session_015yuBJQYfDj95cgX3DaD8SF |
||
|
Yui(ゆい)
|
9f4704a31f | fix(cli): wrap comment not-found errors (#910) |