302 Commits

Author	SHA1	Message	Date
xarmian	9b1a91ab00	feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) (#458) ... * feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) Replaces the temporary handleContentUpdate suppression introduced in TASK-1259 (PR #457) with a proper flush mechanism. Under collab, the Y.Doc + op-log are canonical for live state but items.content needs to stay reasonably fresh for downstream consumers (search index, share-page, exports, plain API readers). ## Mechanism 1. **5s idle timer.** Every editor onUpdate (local OR remote) resets a 5s timer. On fire, PATCHes items.content via the new `?source=collab-snapshot` query param. 2. **Server-side bypass.** handleUpdateItem inspects the source query param. When set, skips the applyContentViaCollab routing entirely and writes directly. Without the bypass, the PATCH would loop back through the applier protocol (the same tab gets asked to apply, acks, server strips input.Content) and leave items.content unchanged forever. The flag is trustworthy because the caller already has edit access. 3. **Dedupe across peers.** Track lastFlushedContent. If our last successful flush already landed this exact markdown, skip the PATCH. Multiple connected tabs would otherwise each fire a redundant flush after every shared edit converges, multiplying server load by the peer count. 4. **On-disconnect flush.** $effect cleanup (item swap or page unmount) calls flushCollabNow(true) BEFORE provider.destroy(). A separate beforeunload listener catches close-tab / reload / external-nav. Both use fetch keepalive: true so the request outlives the page lifecycle. 5. **Item-id race guards.** runCollabFlush captures reqItemId before await; ignores response if item swapped. loadData() clears collabFlushTimer + lastFlushedContent on navigation. ## Files - internal/server/handlers_items.go — accept `?source=collab-snapshot` - web/src/lib/api/client.ts — add api.items.flushCollabContent - web/src/routes/.../[slug]/+page.svelte — handleContentUpdate gains scheduleCollabFlush + runCollabFlush + flushCollabNow; wired to $effect cleanup + beforeunload + loadData reset. Parent: PLAN-1248 * fix(collab): capture ws+itemId at provider mount + apply unescapeDocLinks per Codex review (round 1) Two findings from round 1: 1) [P1] runCollabFlush resolved item.id and wsSlug at execution time, not at schedule time. During item navigation the timer could fire (or $effect cleanup could run) AFTER `item` was already updated to the new item, causing the OLD editor's markdown to be PATCHed against the NEW item's URL — cross-item content corruption. Fix: introduce activeCollabContext = { wsSlug, itemId }, captured at $effect-body time (when the provider is minted). scheduleCollabFlush, runCollabFlush, and flushCollabNow all take their target identity from this captured context, never from live reactive state. Cleared in the $effect's own cleanup (defensive `=== ctx` slot guard so a fast-navigation churn doesn't clobber a successor context). 2) [P2] The disconnect flush read raw editor.storage.markdown .getMarkdown() without unescapeDocLinks, unlike the regular onUpdate path. Closing/navigating before the idle flush could persist escaped wiki links like \[\[TASK-1\]\] which then wouldn't be converted by markdownToWikiLinks. Fix: apply unescapeDocLinks() at the start of runCollabFlush (covers both the timer-driven idle path and the unmount path). * fix(collab): gate UI mutations on foreground+current-item per Codex review (round 2) [P2] runCollabFlush mutated page-scoped state (saveStatus, editorStore.lastSaveTime, lastFlushedContent) before checking whether the captured itemId still matches the foreground item. On item navigation, the cleanup-driven keepalive flush could stamp 'saving' onto the NEW page's saveStatus, leaving it pinned indefinitely (and pollute lastFlushedContent for the new item's dedupe state). Fix: introduce isForegroundCurrent() = !keepalive && item.id === itemId. Gate saveStatus / setLastSaveTime / showSaved on it so background cleanup flushes never touch UI state. Gate lastFlushedContent on item.id === itemId regardless of keepalive so a stale flush can't seed the wrong item's dedupe. * fix(collab): skip cleanup flush on rich→raw transition per Codex review (round 3) [P1] $effect cleanup fires the keepalive flushCollabNow on every provider teardown, including rawMode toggles. The raw-button onclick already pre-populated rawPendingMarkdown with the live editor markdown (which the 1.2s raw debounce will land), so the keepalive PATCH from cleanup is redundant — and worse, can land AFTER the raw save and clobber newer raw edits with the older Y.Doc snapshot. Fix: gate the cleanup flush on `!rawMode`. If rawMode is true at cleanup time, the user just toggled to raw and the raw-mode codepath owns items.content from here. The other cleanup triggers (item nav, canEdit flip, page unmount) all keep firing the flush as before. Note: rawMode === true at cleanup time unambiguously means "transitioning into raw" — the inverse case (already in raw and the cleanup fires for some other reason) is impossible because collabKey gates on !rawMode, so the provider $effect never runs while rawMode is true. * fix(collab): synchronously flush Y.Doc state on rich→raw toggle per Codex review (round 4) [P1] Rich → raw → navigate-without-typing-or-toggling-back never PATCHed the live Y.Doc state to items.content. The previous seed mechanism only set rawPendingMarkdown, which only fires the 1.2s debounce on a subsequent handleRawContentUpdate call — which never happens if the user doesn't type. Fix: await runCollabFlush(ws, itemId, md, true) inside the raw button's async onclick BEFORE flipping rawMode = true. This: - Lands items.content with the live Y.Doc state synchronously (one PATCH, awaited, with keepalive: true so it survives a fast post-toggle navigation). - Seeds lastFlushedContent so any cleanup-driven re-flush is deduped. - Avoids populating rawPendingMarkdown — the raw debounce now only fires for actual user edits in raw mode, eliminating the race where a stale debounce fired after navigation could clobber state. The Round 3 cleanup-skip on rawMode is kept as defense-in-depth (also makes the no-op-when-already-flushed semantics explicit). * fix(collab): loop-flush until stable + cancel timer on rich→raw toggle per Codex review (round 5) Two HIGH findings from round 5: 1) Round 4's single-flush captured md BEFORE the await; concurrent peer edits (e.g. same user's other tab) during the await were lost from the seed and could be overwritten by subsequent raw-mode saves. Fix: loop-flush until stable. Re-read editor markdown after each PATCH; if it changed, flush again. Capped at 3 iterations to bound the transition under aggressive concurrent typing. 2) An onUpdate during the await could schedule a 5s collab flush timer that survived the rawMode flip. The cleanup skipped flushCollabNow on rawMode, but the timer fired its own runCollabFlush — which then PATCHed stale rich markdown on top of subsequent raw saves. Fix: explicitly clearTimeout(collabFlushTimer) at the end of the rich→raw onclick (after the loop-flush, before flipping rawMode). Belt-and-braces with the Round 3 cleanup skip. * fix(collab): seed raw mode from lastFlushed (not unflushed Y.Doc) per Codex review (round 6) [HIGH] Round 5's loop-flush could exit at the 3-iteration cap with md still differing from the last-PATCHed value, then seed rawSeedMarkdown with that unflushed md. An immediate navigation without typing would lose the unpersisted state. Fix: track lastFlushed inside the loop. After the loop, seed rawSeedMarkdown = lastFlushed (the markdown we actually PATCHed), NOT md (potentially a never-flushed in-memory value). If peer edits keep arriving past our cap, items.content lags Y.Doc briefly — but the peer's own 5s flush will catch up shortly, and at least raw mode shows state consistent with items.content rather than holding a value the server never received. * fix(collab): three corner-case fixes per Codex review (round 7) 1) [HIGH] lastFlushed = md was set unconditionally inside the loop-flush, even when runCollabFlush returned false (PATCH failed). rawSeedMarkdown could then be seeded with markdown the server never received. Fix: gate `lastFlushed = md` on runCollabFlush returning true. Failed PATCHes leave lastFlushed at its prior value. 2) [HIGH] lastFlushedContent (the collab-flush dedupe key) was never invalidated by raw-mode direct saves. Scenario: collab flushes A. Raw saves B. User returns to rich + edits back to A. Next collab flush dedupes (lastFlushedContent === A) and skips, leaving items.content stuck on B. Fix: reset lastFlushedContent = null after every successful raw save (both the regular handleRawContentUpdate path and the flushRawIfPending drain loop) so subsequent collab flushes always re-PATCH. 3) [MEDIUM] The async rich→raw onclick applied rawSeedMarkdown + rawMode = true after multiple awaits without verifying the user was still on the same item. A navigation during the loop-flush could let item A's handler resume and seed raw mode on item B. Fix: before mutating component state (rawSeedMarkdown, rawMode), check `item?.id === itemId` (the captured target). Bail with `return` if mismatched. * fix(collab): differentiate flush outcomes + foreground keepalive=false per Codex review (round 8) Two findings from round 8: 1) [P1] runCollabFlush returned `false` for both PATCH failure AND dedupe-skip. The rich→raw toggle treated `false` as "didn't flush" and didn't seed rawSeedMarkdown — but a dedupe means items.content already matches our markdown (the prior successful flush put it there). Raw mode then seeded from the page's stale `item.content` field, and a subsequent raw save could overwrite the current server content with the pre-collab snapshot. Fix: change runCollabFlush's return type to a discriminated string: 'flushed' | 'deduped' | 'failed'. The toggle treats 'flushed' and 'deduped' equivalently for seeding (both mean "server has this markdown") and only bails on 'failed'. 2) [P2] The toggle path used keepalive=true for the awaited flush. Browser keepalive requests can reject for bodies larger than the per-origin keepalive quota (~64KB). On reject, the catch silently fell through and raw mode activated with rawSeedMarkdown null. Fix: switch the toggle path to keepalive=false. The await is synchronous and user-initiated; navigation isn't imminent, so the keepalive escape hatch isn't needed (and risks losing the explicit save). Also added an `aborted` short-circuit so a 'failed' result returns early WITHOUT entering raw mode — user can retry. Cleanup-driven flushes (which DO need to survive page lifecycle) still use keepalive=true.	2026-05-08 22:48:56 -04:00
xarmian	5dc42b60df	feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) (#457) ... * feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) Adds a thin y-websocket-style provider speaking the binary protocol already implemented server-side in internal/collab/room.go. The provider lives in a Svelte 5 .svelte.ts module so connection state (`connected`, `synced`) can be consumed reactively by upcoming UX tasks (TASK-1264 pending-sync indicator, TASK-1265 mobile reconnect). Wire format mirrors the server's first-byte discriminator: 0x00 → y-protocols/sync (persisted to op-log + broadcast) 0x01 → y-protocols/awareness (broadcast only, ephemeral) Lifecycle is bound to the item-detail page via $effect keyed on `${item.id}:${canEdit}` — same key the <Editor> already re-mounts on, so the Y.Doc and provider tear down in lockstep with the editor. View-only viewers (canEdit === false) keep the legacy non-collab editor; their read-only y-binding is deferred to TASK-1266. Reconnect uses 1s/2s/4s/...30s exponential backoff. Sophisticated mobile reconnect (visibility, network state) is TASK-1265. KNOWN TEMPORARY REGRESSION: existing items with non-empty items.content render an empty editor on first open under collab, because the Y.Doc starts empty and TASK-1259 doesn't seed from markdown. TASK-1261 (next in Phase 2) adds the lazy seed-after- initial-sync path. New items + items already round-tripped through collab are unaffected. Drive-by lint cleanup of dead code that escaped Phase 1's make-install-skips-lint loophole: - gofmt -w on internal/collab/{applier,bus,manager}.go - removed unused test/debug helpers Room.peerCount and Room.applierConnCount (re-add with real callers when needed) Parent: PLAN-1248 * fix(collab): gate Editor mount on ydoc + handle applier_request + catch-up state per Codex review (round 1) Three findings from round 1: 1) [P1] $effect constructs ydoc AFTER Editor's onMount runs, so the first mount on an editable item registered StarterKit history instead of the Collaboration extension. The {#key} excluded ydoc, so the editor never re-mounted when ydoc later became truthy → editable users got a non-collab editor while the provider connected to an unused Y.Doc. Fix: gate the editable Editor mount on `ydoc` being ready (`{#if !canEdit} ... {:else if ydoc} ...`). Adds at most one reactive tick of delay; guarantees the first mount has the binding registered. 2) [P1] Provider dropped non-binary WebSocket frames, but the server sends `applier_request` as TextMessage. With TASK-1259 minting active rooms, every concurrent CLI/MCP/API content PATCH would sit blocked for 30s waiting for an ack, then fall back to a direct write — and the in-memory Y.Doc would still hold stale state and clobber it on the next 5s flush. Silent data loss. Fix: parse TextMessage frames as JSON ControlMessage. On `applier_request`, invoke an `onApplierRequest` callback (the page passes `editor.commands.setContent(markdown)`) and send `applier_ack` on success. The ExpiresAtMillis-driven late-apply guard remains TASK-1262's full scope. 3) [P2] Local Y.Doc updates were silently dropped if the socket was closed when handleDocUpdate fired. On reconnect the dumb-relay server can't reconstruct missing updates from a state vector, so any edits made before the first open or during a disconnect could be lost. Fix: after sending syncStep1 in onOpen, also send the current doc state as a single update via `Y.encodeStateAsUpdate(ydoc)`. CRDT idempotency makes this safe on initial open (server already has these ops via op-log replay → sees a no-op update). Larger docs incur a one-time cost on each connection; TASK-1265's mobile-reconnect work can replace this with a buffered queue. * fix(collab): destroy provider during rawMode + enforce ExpiresAtMillis on applier requests per Codex review (round 2) Two findings from round 2: 1) [P1] collabKey ignored rawMode, leaving the WS provider connected while the user edited via RawMarkdownEditor. Raw saves bypass the y-binding (PATCH writes items.content directly), but the server sees an active room → routes the PATCH through the applier flow → no editor mounted → 30s timeout fallback → direct write. The stale Y.Doc still in memory then overwrote the raw save on the next 5s flush after toggling back. Fix: include rawMode in the collabKey derivation so toggling raw destroys the provider (and the in-memory Y.Doc), and toggling back mints a fresh pair that re-seeds from the op-log + TASK-1261's lazy markdown seed. 2) [P1] Provider passed expires_at_millis to the handler but never gated on it. A backgrounded tab that wakes after the server retried or fell back could still apply setContent and overwrite newer peer edits. Fix: enforce the expiry in CollabProvider — check before invoking the handler AND re-check before acking (handlers are awaited and could span the deadline). Suppress the ack if either gate trips; the server interprets "no ack" as "applier unavailable" and falls back cleanly. * fix(collab): prune op-log on direct-write fallback + pre-mutation expiry check per Codex review (round 3) Two findings from round 3: 1) [P1] rawMode toggle to/from rich left a stale op-log: raw saves wrote items.content directly while the destroyed provider's old op-log persisted. Toggling back minted a fresh Y.Doc that replayed the old log → showed pre-raw content → silently overwrote the raw save on the next 5s flush. Fix server-side: when ApplyExternalContent returns ErrNoActiveRoom (no peers in memory, no in-flight Y.Doc state to corrupt), prune the op-log alongside the direct items.content write so future collab sessions start from a clean slate seeded by items.content (TASK-1261's lazy seed). Pruning is intentionally NOT applied to ErrNoApplierAvailable / ErrAllAppliersTimedOut — those paths may have live peers whose Y.Doc state would diverge. 2) [P2] Provider's post-handler expiry check only suppressed the ack, not the actual setContent mutation owned by the page handler. An async handler that crossed the deadline could still write stale markdown into the Y.Doc. Fix: page handler now does its own pre-mutation expiry check inside onApplierRequest before calling setContent. Documented the contract on ApplierRequestHandler — handlers MUST honour expiresAtMillis BEFORE mutating state. * fix(collab): prune op-log on grace-TTL applier-unavailable + suppress autosave when collab active per Codex review (round 4) Two findings from round 4: 1) [HIGH] op-log pruning still skipped ErrNoApplierAvailable. When raw-mode destroys the in-tab provider, the room remains in its 60s grace TTL with zero conns, so the next direct-write PATCH returns ErrNoApplierAvailable (not ErrNoActiveRoom). Stale op-log rows persisted; toggling back within the grace window resurrected pre-raw-save Y.Doc state. Fix: prune op-log on ErrNoApplierAvailable too — the "no live conns" condition makes pruning safe (no peers to corrupt). ErrAllAppliersTimedOut still preserves op-log because peers may still be alive there. 2) [HIGH] Once the WS provider is active the legacy 1.2s content autosave PATCH gets intercepted by the applier path (handleUpdateItem branch added in TASK-1252). On applier success input.Content is nil'd out, so UpdateItem never writes the markdown snapshot. The page's autosave was the only canonical items.content flush in this diff — search / share-page / API consumers would see stale content forever. Fix: short-circuit handleContentUpdate when collabProvider is set. The Y.Doc + op-log are canonical; items.content stays at its pre-collab snapshot until TASK-1260 introduces the proper 5s idle flush with applier-bypass semantics. This is a known Phase-2-internal regression closed by the very next task in this run. * fix(collab): tighten error classification + per-item lock + raw-mode flush per Codex review (round 5) Three findings from round 5: 1) [HIGH] applier.go could return ErrAllAppliersTimedOut even when no applier_request was ever successfully written (a row of write failures followed by no remaining candidates). The handler-side prune skipped that case, leaving stale op-log rows even though no peer received the request. Fix: track `anyWriteSucceeded` across the attempts and return ErrNoApplierAvailable (which prunes) when the loop exits without ever putting bytes on the wire. 2) [HIGH] Race between ApplyExternalContent's no-room classification and the subsequent Prune/UpdateItem: a fresh Join could mint a room and replay the soon-to-be-pruned op-log into a new client, leaving it with stale Y.Doc state that overwrites the freshly-written items.content on the next idle flush. Fix: introduce per-item setup mutex on RoomManager. Join holds the lock across addConn + replayTo and releases it before the long-lived readLoop. New PruneAndApply method wraps the prune+direct-write in the same per-item lock and re-verifies "no live peers" under it (returns ErrRoomActiveDuringPrune if a peer slipped in, in which case the caller falls through to a plain direct write without pruning). Lock order: per-item lock > m.mu > r.mu — Join and PruneAndApply both follow it. 3) [MEDIUM] Raw-mode 1.2s debounce timer could outlive the toggle to rich mode: the deferred PATCH fired post-collab-mint and got routed through the applier path (potentially overwriting newer peer state). Fix: track the latest pending raw markdown in `rawPendingMarkdown`. The Rich-mode button is now an async onclick that awaits a `flushRawIfPending()` synchronous PATCH before flipping `rawMode = false` (which is what activates the collab provider via the collabKey derivation). * fix(collab): evict broken applier conn + retry on prune-race + retain raw pending on PATCH failure per Codex review (round 6) Three findings from round 6: 1) [HIGH] When applier_request write failed, the broken roomConn stayed in r.conns, defeating PruneAndApply's "no live peers" check (which then returned ErrRoomActiveDuringPrune and the handler skipped pruning). Net effect: the prune-safety classification reverted to the round-5 hazard. Fix: in the applier write-failure branch, force-close the conn and call removeConn before continuing to the next applier. Both are idempotent with the readLoop's natural cleanup path (bus.Unsubscribe, conn map delete, conn.Close all tolerate double-invocation). 2) [HIGH] On ErrRoomActiveDuringPrune the handler fell through to a plain direct-write to items.content, bypassing the now-active peer's applier. The peer's stale Y.Doc could still overwrite items.content on the next idle flush. Fix: surface ErrRoomActiveDuringPrune from applyContentViaCollabOnce so the new applyContentViaCollab wrapper can retry the full ApplyExternalContent flow against the freshly-active room. Capped at applyContentMaxRetries=3 to prevent runaway loops if joins keep landing during prune attempts. After exhaustion, returns the same sentinel — the handler's existing `if err == nil { input.Content = nil }` gate falls through to direct write, which is the correct degraded-mode behavior. 3) [MEDIUM] flushRawIfPending cleared rawPendingMarkdown before the PATCH succeeded and the Rich-mode toggle always set rawMode = false regardless of flush outcome. A failed flush could activate collab with unsaved raw edits. Fix: rework flushRawIfPending to return success bool, retain rawPendingMarkdown on PATCH failure, and gate the Rich-button transition on `ok`. Added a re-entrancy guard (rawFlushInFlight) so a rapid double-click waits for the in-flight flush to settle instead of issuing a duplicate PATCH. * fix(collab): drain-loop flushRawIfPending to handle fast-typist edge per Codex review (round 7) [P1] flushRawIfPending snapshotted rawPendingMarkdown then awaited the PATCH; if the user typed during the await, the equality check preserved the newer edit but the function still returned `true` and the Rich-mode handler flipped collab on. The newly-active provider then raced the un-flushed pending raw save — exactly the hazard the guard is meant to close. Fix: rework flushRawIfPending into a bounded drain loop. Each iteration snapshots-PATCHes-clears (with the equality check). The loop runs up to RAW_FLUSH_DRAIN_CAP=5 iterations, returning `true` ONLY when rawPendingMarkdown is null on exit AND no PATCH failed. A fast typist who keeps the queue non-null across the cap returns `false`, leaving the user in raw mode (next click retries). PATCH failure short-circuits with `false` so the toggle stays in raw mode and the unsaved markdown is preserved for retry. * fix(collab): atomic prune+content-write + preserve newer raw edit on stale PATCH response per Codex review (round 8) Two findings from round 8: 1) [P1] PruneAndApply ran the op-log prune under the per-item lock but the items.content write happened later in the post-loop UpdateItem call, OUTSIDE the lock. A fresh Join landing in that gap could replay the now-empty op-log, mint a peer with stale Y.Doc state, and then overwrite the freshly-written items.content on the next idle flush. Fix: applyContentViaCollab now takes a `directWrite` callback that the caller (handleUpdateItem) implements as a content-only UpdateItem. PruneAndApply's applyFn invokes it AFTER the prune so both run inside the same per-item critical section. The trade-off is two DB round-trips when a PATCH carries content + other fields together (rare): the content-only update happens inside the lock; the rest (title, fields, status) flows through the post-loop UpdateItem with input.Content nil'd to suppress the duplicate write. 2) [P1] In flushRawIfPending's drain loop, `item = updated` assigned the server-side snapshot from the just-PATCHed markdown even when a newer raw edit had landed in the meantime. RawMarkdownEditor mirrors `item.content` into its textarea unconditionally (line 16), so the stale assignment would reset the textarea mid-keystroke and lose the queued edit. Fix: only swap in the full updated snapshot when `rawPendingMarkdown === markdown` (no newer edit). Otherwise keep our local content and adopt only the server-side metadata (timestamps, version, modified_by) via spread. * fix(collab): atomic mixed PATCH + raw autosave stale guard + rich→raw seeding per Codex review (round 9) Three findings from round 9: 1) [P1] Toggling FROM rich+collab TO raw mode seeded RawMarkdownEditor from items.content, which is intentionally stale under collab (handleContentUpdate is suppressed while the provider is connected; TASK-1260 closes that gap with a 5s flush). Saving from raw mode would overwrite the live Y.Doc state with a pre-collab snapshot. Fix: when toggling to raw with a connected provider, capture the editor's current Y.Doc-derived markdown via `editor.storage.markdown.getMarkdown()` into a one-shot `rawSeedMarkdown` slot and pre-populate `rawPendingMarkdown` so the first auto-save persists it. RawMarkdownEditor seeds from `rawSeedMarkdown ?? item.content`. Cleared on rich-mode toggle. 2) [P1] The regular debounced raw autosave still assigned `item = updated` from a stale PATCH response. Same stale-snapshot hazard the Round 8 fix closed in flushRawIfPending. Fix: equality-check `rawPendingMarkdown === toSave` before swapping in the server snapshot. On stale, keep local content and adopt only the server-side metadata via spread. 3) [P2] Round 8 split the items.content write (under per-item lock) from the rest of UpdateItem (post-loop), losing atomicity for mixed PATCHes (content + title) and breaking Store.UpdateItem's content-versioning peek at Title. Fix: directWrite callback now invokes the FULL UpdateItem inside the per-item lock. A `fullWriteHandled` flag tells the handler to skip the post-loop UpdateItem entirely (otherwise we'd duplicate the write and create two version-history rows). Mixed PATCHes are atomic again under the lock. * fix(collab): clear raw seed/pending on item navigation per Codex review (round 10) [P1] Navigating between items left rawSeedMarkdown, rawPendingMarkdown, and the contentDebounceTimer set from the previous item. This caused two concrete hazards: (a) Item B's raw editor mounted with item A's live markdown via `rawSeedMarkdown ?? item.content`. (b) Clicking Rich on item B fired flushRawIfPending which PATCHed A's queued markdown INTO item B (cross-item data bleed). Fix: at the top of loadData(), clear contentDebounceTimer, rawSeedMarkdown, and rawPendingMarkdown so each navigation starts from a clean slate. The collab provider's own lifecycle is already keyed on item.id via $effect cleanup, so it doesn't need the same explicit reset. * fix(collab): item-id race guard on raw PATCH responses per Codex review (round 11) [P1] In-flight raw PATCH responses (debounced autosave AND drain loop) could clobber a newly navigated item. Clearing contentDebounceTimer in loadData only cancels timers that have not fired; an awaiting fetch keeps running and its `.then` / `.catch` would assign back to the new page's `item` state. Fix: mirror the existing TASK-754-style race guard pattern (already used in the SSE / sync handlers above). Capture `reqItemId = item.id` BEFORE the PATCH, then in the response handler bail if `!item || item.id !== reqItemId`. Applied to both handleRawContentUpdate's setTimeout body and flushRawIfPending's drain loop. * fix(collab): reset saveStatus on item navigation per Codex review (round 12) [P2] After Round 11's race guard, a stale raw PATCH response that matched a now-different item.id was correctly discarded — but saveStatus had already been set to 'saving' before the await. With loadData not resetting it, the next item could mount with saveStatus pinned at 'saving' indefinitely, which then suppressed all SSE/sync refreshes via the `if (saveStatus === 'saving')` guards above. Fix: in loadData's per-item state reset, clear saveStatusTimer and reset saveStatus to 'idle' alongside the other transient state. Cheap, scoped, no impact on the in-flight save's eventual discard path.	2026-05-08 20:48:57 -04:00
xarmian	50e0936b34	feat(collab): designated-applier protocol for external content updates (TASK-1257) (#455) ... * feat(collab): designated-applier protocol for external content updates (TASK-1257) The keystone task for CLI / API / MCP integration during co-edit sessions. When a content update arrives via PATCH while at least one browser tab is connected to the item's collab room, the server can't write items.content directly — the connected tabs would silently overwrite it on the next 5s idle flush using their (now stale) Y.Doc state and the caller's update would be lost. Solution: nominate one connected tab as the "designated applier", send it a JSON control message with the new markdown, the browser does editor.commands.setContent(markdown) which the y-tiptap binding translates into Y.Doc updates that propagate via the regular sync path. Items.content gets refreshed via the next 5s flush (TASK-1261). Architecture: internal/collab/applier.go (new): - ControlMessage struct — JSON envelope for applier_request / applier_ack frames. Carried over WebSocket TextMessage, which is unambiguous against y-protocol's BinaryMessage. - ApplyExternalContent(itemID, markdown) — public entry point. Returns nil on ack, ErrNoActiveRoom when there's no room (caller falls back to direct write), ErrNoApplierAvailable when the room has no live conns, ErrAllAppliersTimedOut when every attempt expired. - Election: pickApplier returns the longest-connected roomConn that hasn't already been tried, with deterministic tiebreak on conn id. Stable choice — longest connection has the most authoritative cumulative Y.Doc state, fewer flicker risks. - Retry loop: applierMaxAttempts=2, applierFirstTimeoutVar=30s, applierRetryTimeoutVar=15s. The Var-suffixed names exist so test helpers can shrink to ms without sleeping a real minute. - Pending-ack tracking: per-room map[requestID]*pendingApplierAck pairing the channel a PATCH handler is waiting on with the conn the ack is expected from. expectedConn check prevents an unrelated peer from spoofing acks for someone else's request. internal/collab/room.go (extended): - roomConn gains connectedAt for the election. - readLoop branches TextMessage → handleControlMessage which decodes the JSON and routes applier_ack to the room's pending tracker. Unknown control types and malformed JSON are silently dropped so a bad client can't break the loop. internal/collab/manager.go: - Registers connectedAt on Join. - Initialises room.pendingAcks alongside conns map. internal/server/handlers_items.go (extended): - handleUpdateItem now branches on input.Content != nil + s.collab != nil: routes through s.applyContentViaCollab; on success, zeros input.Content so UpdateItem's direct write is suppressed. - Field-only PATCHes skip this branch entirely — backward-compatible. internal/server/handlers_collab.go: - applyContentViaCollab wraps mgr.ApplyExternalContent with per-error-class slog warnings so operators can see degraded paths (timeouts → warn; no-room / no-applier → quiet, the common case for non-co-edit CLI updates). - actorIDFromRequest helper for log fields. Tests (5 new): - TestApplyExternalContentNoActiveRoom — sentinel error path. - TestApplyExternalContentHappyPath — applier echo acks within ms. - TestApplyExternalContentTimeoutsThenFails — applier never acks; we hit applierFirstTimeoutVar then ErrAllAppliersTimedOut. - TestApplyExternalContentTimeoutThenSecondAcks — first applier silent, retry picks second-longest-connected, succeeds. - TestApplyExternalContentRejectsAckFromUnexpectedConn — defence- in-depth: peer B forges an ack for peer A's request; the room's expectedConn check rejects it; ApplyExternalContent runs to timeout instead of being satisfied by the forgery. All tests pass under -race. Full suite green. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clean pendingAcks on success + expires_at on applier_request per Codex review (round 1) P2 #1: ApplyExternalContent retained the per-request pendingAcks entry on success. Each successful external update therefore leaked a request_id + channel + expected-conn pointer for the remainder of the room's lifetime — across long-lived sessions the map would grow without bound. Add cancelPendingAck to the ack-success path so the entry is released as soon as the request completes. Test added (TestApplyExternalContentCleansPending- AcksOnSuccess) drives 5 successful applies and asserts the pendingAcks map is empty afterwards. P2 #2: applier_request had no client-enforceable expiry, so a backgrounded tab could process a stale request 60s later and overwrite newer edits with old markdown after the server had already retried with a different applier (or fallen back to direct write). Add ExpiresAtMillis to the ControlMessage envelope, populated per attempt with `now + timeouts[attempt]`. The browser-side handler (TASK-1263) is responsible for the client-side Now() check before applying — without that check the field is documentation-only. Added test (TestApplyExternalContentSendsExpiresAt) regression-tests the server stamp. Server-side cleanup is also reinforced: cancelPendingAck on timeout (already present) means a late ack from a timed-out applier is rejected at the room layer (entry is gone). The expires_at_millis is the second line of defence for the case where the browser sends the Y.Doc setContent BEFORE the ack — the request must not be applied at all.	2026-05-08 16:30:39 -04:00
xarmian	79eb00d2a1	feat(collab): periodic auth revalidation timer (TASK-1256) (#454) ... * feat(collab): periodic auth revalidation timer (TASK-1256) Catches mid-session revocations on a live collab WebSocket the same way handlers_events.go's sseSubscriberStillHasAccess does for SSE. When the WS handler upgrades, it spawns a goroutine that ticks every collabMembershipRevalInterval (60s, jittered across [0, interval) on first fire to avoid post-deploy reconnect-storm spikes). Each tick re-runs authorizeCollabAccess — the same workspace-access ladder used at upgrade time, including the "fresh-fetch user from store" semantics that make admin-demoted-mid-stream visible without waiting for the next request. On access loss the handler routes through a new RoomManager.CloseConn(itemID, conn, code, reason) method which: - Looks up the roomConn in the manager so the close frame can go out under the per-conn writeMu (no concurrent-write panic against the room's writeLoop or replay path). - Sends a websocket.ClosePolicyViolation frame with a human-readable reason ("Your access to this item was revoked.") so the frontend can stop reconnecting in a tight loop. - Falls back to plain conn.Close when the conn isn't tracked yet (race window between Join's getOrCreate and addConn). The goroutine is bound to the handler's lifetime via a `stop` channel that closes when handleCollab returns; no leaked timers or goroutines after disconnect. Test (TestCollabMembershipRevalidationClosesOnRevoke): - Shrinks the reval interval to 30ms so the test runs in tens of ms rather than 60 seconds. - Bootstraps an admin (so the no-users escape hatch is closed), creates a non-admin member user, mints a session, dials in. - Calls RemoveWorkspaceMember while the WS is open. - Asserts the next read returns an error (close frame or transport failure — both are acceptable signals the server tore the connection down). Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): WriteControl for revoke close + tighten test failure modes per Codex review (round 1) P2 #1: CloseConn used rc.writeMessage which acquires the per-conn writeMu. If the room's writeLoop / replay was mid-WriteMessage to a slow peer, revocation would block behind that writer and never force-close the unauthorized conn. Switch to conn.WriteControl, which gorilla documents as concurrency-safe with normal writes (it bypasses the conn's normal write path) and accepts an explicit deadline so a stuck send can't extend the budget indefinitely. The deadline is 1s — generous for a healthy conn, short enough that a half-broken socket falls through to plain Close quickly. The itemID parameter stays in the API for symmetry / future per-room metrics, but is no longer used for the actual close path now that the writeMu lookup is gone. P2 #2: TestCollabMembershipRevalidationClosesOnRevoke previously treated a read-deadline timeout as a log-only branch — the test could pass after waiting 2s with the WS still open, exactly the bug being regression-tested. Restructure to fail fast on timeout (t.Fatalf isTimeout(err)) and prefer ClosePolicyViolation as the expected close code, falling back to "any non-timeout error" only because the underlying TCP teardown can produce different error shapes depending on timing. The isTimeoutOrEOF helper that masked the failure is replaced with a narrowly-scoped isTimeout. * fix(collab): distinguish access denial from transient errors in reval per Codex review (round 2) P-MEDIUM: the revalidation goroutine treated any non-nil error from authorizeCollabAccess as revocation, including transient store errors (GetUser / GetWorkspaceByID / grant-lookup blips). One DB hiccup would close every active collab WS with ClosePolicyViolation, which is a worse UX than the bug being guarded against. Distinguish via errors.As against *statusError (the typed return from authorizeCollabAccess used for all "we know they don't have access" branches). Plain errors fall through to a warn-level log + timer reset so the next tick retries. Three branches in the revalidation switch now: err == nil still authorised — reset timer. isAccessDenial(err) real revocation — close conn with typed reason. default transient — log warn, keep conn open, reset. * fix(collab): re-fetch item on each reval tick per Codex review (round 3) P2: revalidation re-authorized against the *Item captured at upgrade time, so an item moved to a collection the user can't see — or hard-deleted — would not be caught: authorizeCollabAccess kept checking the stale CollectionID, kept passing, and the WS stayed open against an item the user no longer has access to. Re-fetch via s.store.GetItem(itemID) at the start of each tick: - error → log warn, keep conn open, retry next tick (matches the transient-store-error policy from round 2). - nil → item hard-deleted (or never existed): close with ClosePolicyViolation + "This item is no longer available." - otherwise → authorize against the FRESH item, picking up any collection move automatically. Per-tick GetItem is one indexed lookup per minute per active connection — negligible compared to the auth-cascade GetUser / member / grant queries that already run on the same tick.	2026-05-08 16:07:26 -04:00
xarmian	e7b1c3b5ae	feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453) ... * feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a working dumb-relay collab server. Per-item Room created lazily on first Join, kept alive across transient disconnects via a 60s grace TTL, reclaimed when the grace expires with no fresh subscribers. Components: - internal/collab/room.go — Room struct + lifecycle · roomConn pairs (id, conn, bus channel, write mutex). The id is server-assigned per WS so writeLoop can suppress own-event echoes without decoding the Y.Doc to read the Yjs ClientID. · readLoop discriminates yMessageSync vs yMessageAwareness on byte 0. Sync frames are persisted to the op-log AND broadcast; awareness frames are broadcast only (presence is ephemeral). Persistence happens BEFORE broadcast so a crash mid-publish loses at most a live keystroke that the originator will replay on reconnect anyway. · writeLoop drains the bus subscription and writes non-self events to the WS, gated by a per-conn write mutex (gorilla's "one writer at a time" rule). · removeConn arms a 60s graceTimer when the last conn drops; a fresh addConn cancels the timer. onGraceExpired re-checks len(conns) == 0 under the room mutex and only THEN sets closing=true + calls back to the manager. The race between "manager.getOrCreate found us" and "grace timer fired" is handled by addConn returning errRoomClosing; the manager retries via getOrCreate which mints a fresh Room. - internal/collab/manager.go — RoomManager + RoomManagerConfig · NewRoomManager wires production defaults (DefaultGraceTTL = 60s, DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an explicit config so tests can drop graceTTL to a few ms without sleeping a minute. graceTTL is per-manager, not a package var, so parallel tests with different TTLs don't trip the race detector. · Join is the public entry point: getOrCreate → addConn (with retry on errRoomClosing) → replayTo → spawn writeLoop goroutine → run readLoop inline → wait for writeLoop drain → return. The inline read keeps the HTTP handler in scope so its `defer conn.Close()` doesn't fire until both loops exit. · Close is for graceful server shutdown — closes every active conn under the room mutex, then drains the manager's room map. - internal/collab/manager_test.go — 7 tests covering: lazy create, op-log replay-on-connect (two seed rows arrive in order), sync broadcast + persist (peer B sees A's frame, originator does not echo, op-log gains a row), awareness broadcast WITHOUT persist, cross-item isolation (item-a frames don't leak to item-b subscribers), grace-TTL reclaim with a 50ms config TTL, grace cancel on reconnect within window, manager.Close shuts down every active conn. All tests run with -race; the bus's concurrent-publish test was already covered by TASK-1253. - internal/server/handlers_collab.go — wire to RoomManager · Returns 503 when s.collab is nil (matches the SSE handler's "events bus not configured" 503 — fail loud rather than silently accept the upgrade). · Otherwise hands the upgraded conn to s.collab.Join, which blocks until the WS closes. Unexpected close codes get the same warn-log as before; normal closures stay quiet. - internal/server/server.go — adds *collab.RoomManager field + SetCollabRoomManager setter (nil-safe optional, like SetEventBus). - cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into the running server alongside the event-bus wiring. Single-instance only today; multi-replica fanout via Redis is a deferred IDEA per the Plan body. - internal/server/handlers_collab_test.go — adds testServerWithCollab helper (so existing collab tests get a real RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager which asserts the 503 path for unwired servers. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1) P1 — concurrent peers raced AppendYjsUpdate, violating the single-writer-per-item contract documented on the store call. Each peer's readLoop runs in its own goroutine, so two peers in the same room could call AppendYjsUpdate concurrently. On Postgres that risks the BIGSERIAL allocation-vs-commit-order cursor gap that TASK-1252's contract was specifically guarding against. Add an appendMu on Room held across the persist+publish sequence; reads, awareness frames, and OTHER rooms remain unserialised. Regression test (TestRoomManagerSerializesSyncAppends) drives 4 peers × 10 writes concurrently and asserts the op-log gains exactly 40 rows. Without appendMu this would intermittently surface fewer rows or out-of-order ids on Postgres; with it the count is deterministic and the race detector stays clean. P2 — Server.Stop did not close s.collab. Active collab WS goroutines + grace timers could keep using s.store after the server's other cleanup paths winding down. Add s.collab.Close() before rateLimiters.Stop so any Join goroutines holding rate-limiter handles can wind down cleanly. nil-safe via the existing collab optional-attachment pattern. * fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2) P2: a joining peer subscribed to live events BEFORE its writer goroutine started. During a long replay, live sync events would pile up in the 64-event bus channel; once full, MemoryOpBus.Publish silently drops them, leaving the new peer connected but permanently missing those updates. Restructure runConn to spawn the writer goroutine FIRST so it drains the bus subscription concurrently with the replay. Both replay and writer go through rc.writeMessage, which holds the per-conn write mutex, so we never violate gorilla's one-writer-at-a-time rule. Yjs CRDTs are commutative — applying live op 100 before replay op 50 yields the same final Y.Doc as the reverse order — so interleaving is correct. The trade-off is a brief "out of causal order" UX wobble during replay, which is acceptable: the alternative would require either an unbounded queue or losing updates the way the original order did. * fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3) P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never srv.Stop. http.Server.Shutdown does NOT terminate hijacked connections (WebSockets), so active collab sessions kept running until process exit and could race the deferred store close. The RoomManager.Close path added in round 1 only fires inside Stop, so without this call the production shutdown was effectively bypassing the new cleanup. Add srv.Stop() after srv.Shutdown in the serveCmd shutdown sequence. Stop also runs the existing background-loop teardowns (orphan GC, MCP audit writer, MCP session tracker) which were previously already part of Stop's contract — those will continue to fire as they always have, so this commit's only behavioural change is "now also closes the collab room manager". * fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3) P1 — RoomManager.Close was not a true drain barrier. closeAll closed the WebSockets but did NOT wait for the corresponding Join goroutines (running runConn) to exit. Server.Stop returned before in-flight collab work finished, racing the deferred store close on process exit. Fix: track every Join in m.activeJoins (sync.WaitGroup); Close iterates closeAll first (waking up every reader by closing the conn), then activeJoins.Wait — guaranteeing no collab goroutine is still running by the time Close returns. P2 — replay-time bus overflow could still drop sync events on a slow drain (writeLoop blocks on the same writeMu replayTo holds, so a long replay starves the bus drain even with the writer goroutine started before replay). Two-part response: (a) Bump the per-subscriber bus channel buffer from 64 to 256. Sized for a 5x safety margin on a 1k-row replay against a chatty 5-peer room (~50 events/sec during a ~1s replay). (b) The architectural fix — force-close subscribers on overflow, honoring the bus's documented slow-peer recovery contract — is filed as TASK-1273 follow-up. That requires extending the OpBus interface (per-subscriber drop callback or counter) and an active health-check tick in the room manager; both are out of scope for TASK-1255's "lazy room + grace TTL" deliverable. For PLAN-1248's single-instance scope and typical editor load, 256 covers realistic workloads. Pathological / load-test scenarios exposing overflow can recover via Yjs's state-vector negotiation on reconnect, and TASK-1273 will tighten that to an active kick. * fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4) P2: http.Server.Shutdown does NOT wait for hijacked WebSocket handlers, so a Join() call from a freshly-upgraded conn could fire AFTER Close() returned. The previous Add-then-Wait pattern was correct for already-started Joins but couldn't catch a Join that hadn't yet hit Add when Close fired. Race: Close iterates the (empty) rooms map, Wait sees zero waiters, Close returns; THEN Join hits Add and proceeds against a torn-down store. Add a `closed` flag gated by the same mutex that wraps activeJoins.Add. Three orderings, all safe: 1. Add before Close.closed=true → Wait blocks until Done. 2. Close.closed=true before Add → Join sees closed=true under the same lock and returns errManagerClosed without ever incrementing the WaitGroup. 3. Close called twice → second call short-circuits (idempotent). getOrCreate also gets a closed-flag short-circuit so a future caller can't bypass the gate by skipping Join. Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close Join returns errManagerClosed, plus a second Close() is a no-op. All 15 collab tests pass under -race.	2026-05-08 15:38:45 -04:00
xarmian	2945ee27dd	feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) (#452) ... * feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) WebSocket entry point for Yjs-based collaborative editing on a single item under PLAN-1248. Bare-bones in this PR by design: upgrade + log connect/disconnect + drain reads. Protocol logic (forwarding to OpBus, persisting to op-log, awareness fan-out) arrives in TASK-1255 (room manager). Authorisation mirrors RequireWorkspaceAccess but keyed on the item's workspace ID rather than a {slug} URL param — the WS URL only carries itemID. Implementation re-uses the same access ladder: fresh-install escape hatch (no users) → grant legacy workspace-scoped API token, no user → grant if token's workspace matches the item's workspace OAuth token allow-list (TASK-953) → reject when workspace not on consented list authenticated user → admin OR member OR has guest grants User is re-fetched from the store on each upgrade (not trusted from session-context cache) so a mid-session admin demotion or member removal closes the upgrade path immediately. Mirrors sseSubscriberStillHasAccess. Periodic per-connection revalidation lives in TASK-1256. Route registered alongside SSE (outside the jsonContentType middleware group, but inside the auth middleware chain). Promotes github.com/gorilla/websocket from indirect to direct dep and bumps to v1.5.3 (latest stable; v1.5.0 was already in go.mod transitively via another package). Tests cover: - fresh-install escape hatch grants the upgrade - bootstrapped server rejects unauthenticated upgrade with 401 - non-member with valid session is rejected with 403 (NOT 401 — confirms the access path runs after auth, not before) - unknown item surfaces as 404 (not 401/403 leak) - empty itemID segment doesn't match the route Test infrastructure note: dialCollab takes an explicit User-Agent because pad's session-binding middleware hashes the UA at CreateSession time and re-checks on every request — the dialer must match what was stored, otherwise the cookie is rejected before the workspace check fires (and we'd see a misleading 401 where 403 was expected). Parent: PLAN-1248. Phase 1 — Backend foundation. * style: gofmt handlers_collab_test.go per Codex review (round 1) * fix(server): SetReadLimit + nginx upgrade headers for collab WS per Codex review (round 2) P-MEDIUM #1: handleCollab.ReadMessage had no per-message size cap, so an authenticated client could send an arbitrarily large frame and force unbounded server-side buffering — the HTTP body limit applied by the auth chain doesn't apply once the connection is upgraded. Set SetReadLimit(1 MiB), generous for keystroke-rate Yjs ops and large enough for a typical initial-sync state. ReadMessage returns an error when exceeded, which the existing read loop handles as a normal close. P-MEDIUM #2: deploy/nginx.conf routed /api/v1/collab/ through the default `location /` block, which sets `Connection ""` (cleared so HTTP keepalive works) — that strips the Upgrade header, so WebSocket upgrades silently fail behind the documented nginx deployment. Add a dedicated location block with proxy_set_header Upgrade $http_upgrade / Connection "upgrade", same 24h read/send timeouts as SSE so an idle editor tab does not get cut off mid-session. * fix(server): enforce per-item visibility in collab WS upgrade per Codex review (round 3) P2: authorizeCollabAccess granted upgrade to any workspace member or guest-with-grants without checking whether THIS specific item was visible to that user. A restricted member (collection_access=specific) or a guest with grants on item A could upgrade /api/v1/collab/{itemID} for an item B in a different collection — they'd see live edits to a document they have no right to read. Restructure the access ladder: 1. Workspace-level gate stays as-is: "any access at all?" If no membership AND no grants → 403 (unchanged). 2. Item-level visibility check added on top, mirroring requireItemVisible without depending on middleware-set request context (the WS path doesn't go through RequireWorkspaceAccess): - VisibleCollectionIDs nil → "all" access → grant. - Item's collection in the visible set → grant. - Item-level grant on this exact item → grant (covers guests given access to a single item rather than a whole collection). - Else → 404, mirroring requireItemVisible's "don't leak existence" pattern. Admin path returns nil before this check, so no change there. Legacy workspace-scoped API tokens grant editor-equivalent access on workspace match (predates the grants design); that branch is untouched since legacy tokens don't have a user identity to scope per-item grants against. Test added: TestCollabUpgradeRejectsRestrictedMemberForeignCollection — member with specific access to collA tries to upgrade for an item in collB → 404. Existing 5 tests still pass. * fix(server): strict per-item visibility check + sibling-grant test per Codex review (round 4) P1 (round 4): VisibleCollectionIDs is broader than full-collection access — it includes collections "anchored" by an item-level grant (so the nav can still surface the parent collection of a granted item). Round 3's check treated every visible collection as full access; a guest with grant `item:A` could upgrade /api/v1/collab/{B} for a sibling B in the same collection. Tighten by mirroring guestResourceFilter / requireItemVisible: 1. Coarse stage stays — collection must be in the visible set. 2. NEW strict stage when the user has item-level grants: (a) full collection grant on this collection → grant (b) member's "specific" access list including this collection → grant (c) item grant on THIS exact item → grant Else → 404 (the visible-set hit was anchored by a sibling's grant, not by full collection access). When the user has NO item grants, the coarse-only check is sufficient — visibility came from full collection access (member's "specific" list, full collection grant, or "all" access). Test added: TestCollabUpgradeRejectsGuestWithSiblingItemGrantOnly — guest with item:A grant tries to upgrade for sibling B in the same collection → 404 (the bug being regression-tested) AND verifies the granted item A still upgrades cleanly to 101 Switching Protocols.	2026-05-08 14:36:47 -04:00
xarmian	264f5b0041	feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253) (#451) ... * feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253) New internal/collab package for the dumb-relay collab server in PLAN-1248. Defines the OpBus pub/sub interface and ships MemoryOpBus, the in-process implementation used by every shipping target today (single-binary self-host, single-replica pad-cloud). OpBus shape mirrors internal/events.MemoryBus so a future RedisOpBus is a drop-in for multi-replica deployments — that's filed as a separate IDEA at PLAN-1248 close, since the dumb-relay design intentionally keeps Redis off the self-host dependency surface. OpEvent carries: - ItemID fan-out filter - ClientID Yjs client id, used by designated-applier election (TASK-1257); the bus itself does not interpret it - Type "sync" (Y.Doc binary update — persisted by the room manager) or "awareness" (cursor/presence — broadcast only, never persisted) - Data raw y-protocol message; opaque to the server - Timestamp UnixMilli, auto-stamped on Publish MemoryOpBus semantics: - 64-event buffered subscriber channels (matches internal/events default — sized against keystroke-rate workload). - Non-blocking Publish: a slow subscriber whose channel is full has events DROPPED with a warn log rather than back-pressuring the broadcast loop. The room manager (TASK-1255) is responsible for closing genuinely unhealthy peers; the bus only protects itself. - Idempotent Unsubscribe (no panic on double-unsubscribe). - Close clears the subscriber map and closes every channel under the same write lock that gates Publish, so a final inflight Publish can't race a Close into delivering on a closed channel. Tests cover: subscribe/publish fan-out per item filter, unsubscribe closes the channel, slow-consumer drop without blocking, accurate SubscriberCount across subscribe/unsubscribe, Close cleans up every channel regardless of itemID, and concurrent-publishers race-clean under -race (small drop count tolerated — that's the slow-consumer contract; an exact-delivery test would defeat its own purpose). No external dependencies beyond stdlib + slog. RedisOpBus stub is intentionally NOT included — separate IDEA per the Plan body. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clone OpEvent.Data + document recovery contract per Codex review (round 1) P1 — Sync-op drops were undocumented as recoverable. Sync drops ARE recoverable in the dumb-relay design: the room manager (TASK-1255) appends to the op-log BEFORE Publish, so any peer that misses a sync op via channel-full drop can replay since their last cursor on reconnect (TASK-1252's LoadYjsUpdatesSince + Yjs state-vector negotiation). The room manager is responsible for detecting slow channels and force-closing the owning WebSocket, which kicks the peer into a fresh reconnect + replay. The bus does not take that action itself because it has no concept of which peer owns which channel — that mapping is the room manager's domain. Doc comment now spells this out explicitly. P2 — OpEvent.Data is a []byte; the same slice header was queued to every subscriber, so a publisher's later buffer reuse OR any subscriber's mutation could corrupt the bytes other subscribers observe. gorilla/websocket's ReadMessage is allowed to reuse its read buffer between messages, so this hazard is real for the production publisher (the WS handler in TASK-1254). Clone Data once at the publish boundary; document the per-receiver immutability expectation in the same comment block. No behavioral test change — the existing slow-consumer-drop test still passes; the clone path adds one allocation per Publish but nothing observable to callers beyond the immutability guarantee.	2026-05-08 13:56:21 -04:00
xarmian	04514817ae	feat(store): add Yjs op-log table + store methods (TASK-1252) (#450) ... * feat(store): add Yjs op-log table + store methods (TASK-1252) Persistence groundwork for the dumb-relay WebSocket server in PLAN-1248. The item_yjs_updates table records every Yjs binary update (browser edits, future designated-applier conversions of CLI/API content changes) so reconnecting peers can replay updates since their last known cursor and cold rooms can rebuild their in-memory Y.Doc. Schema (mirrored across SQLite + Postgres): - id monotonic — INTEGER PRIMARY KEY AUTOINCREMENT (SQLite) / BIGSERIAL (Postgres). Never reused, even after deletes; serves as the cursor every reconnecting client compares against. - item_id FK with ON DELETE CASCADE so item deletion reclaims op-log space automatically. - update_data raw Yjs binary update — BLOB / BYTEA. Opaque to the server. - schema_version stamped per row. Mismatch on connect drives TASK-1268's snapshot-and-rebuild flow. - created_at ISO8601 UTC TEXT, matching pad's cross-dialect timestamp convention (see migrations/047_attachments). Drives PruneYjsUpdatesBefore. Store API (internal/store/yjs_updates.go): - AppendYjsUpdate — validates non-empty itemID/data/schemaVersion, inserts and returns the new monotonic id (RETURNING on Postgres, LastInsertId on SQLite). Empty-zero-byte updates are rejected at the Go layer rather than relying on NOT NULL — they're a no-op that would only pollute the log. - LoadYjsUpdatesSince — strict id > sinceID filter, ordered by id ascending. sinceID=0 returns everything (cold-room rebuild path). Tolerates either RFC3339 or "YYYY-MM-DD HH:MM:SS" timestamp formats on read so any future operator-written / CURRENT_TIMESTAMP-style row doesn't blow up the load path. - PruneYjsUpdatesBefore — created_at < cutoff, scoped to itemID. Returns rows-affected count. Used by the eventual GC sweeper (out of scope for this task). Tests cover: append + monotonic ids, load-since-cursor filtering, input validation, prune scoped to itemID, and ON DELETE CASCADE on parent item removal. Pass on SQLite locally; Postgres mirror migration + store methods are dialect-agnostic. Parent: PLAN-1248. First task of Phase 1 — Backend foundation. * docs(store): document AppendYjsUpdate per-item serialization contract per Codex review (round 1) P1: Postgres BIGSERIAL ids are allocation-ordered, not commit-order. Concurrent appends to the same item could in theory produce a cursor gap — a slower transaction can hold a smaller id while a faster one commits a larger id first, and a reader that advances past the visible larger id would later miss the smaller id when it commits. The dumb-relay room manager (TASK-1255) is the sole writer per item by design — there's exactly one goroutine appending per Y.Doc — so the hazard does not manifest in practice. The fix is at the API contract level: the doc comment now spells out the serialization requirement, why the room manager satisfies it, and the multi-replica re-enforcement note for the future Redis-fanout IDEA. We do not take an internal advisory lock because that would be paid by every append even though the caller already holds the per-room mutex. No code change — contract is at the doc comment.	2026-05-08 13:28:55 -04:00
xarmian	d915cc3cf8	feat(cli): per-server credentials in credentials.json with v1 → v2 migration (TASK-1228) (#435) ... Implements IDEA-1226. ~/.pad/credentials.json is now a map keyed by server URL so one developer machine can stay logged in to multiple Pad instances simultaneously — `apm/` repo on Pad Cloud, `target/` repo on local, `testing/` repo on staging — without each `pad init --url <other>` clobbering the previous server's credentials. ## On-disk format v2 (new): { "version": 2, "credentials": { "https://app.getpad.dev": {"token": "...", "user_id": "...", ...}, "http://127.0.0.1:7777": {"token": "...", "user_id": "...", ...} } } v1 (legacy, read-only): {"server_url": "...", "token": "...", "user_id": "...", ...} Reads transparently migrate v1 → v2 in memory; writes always emit v2. Side-effect-free reads — the on-disk file stays v1 until login/logout/ setup triggers a Save, which is when migration becomes durable. This keeps `pad <read-only-command>` from rewriting credentials.json on every invocation just because the binary upgraded. ## API Replaces the three top-level helpers (LoadCredentials / SaveCredentials / DeleteCredentials) with a CredentialStore type: - LoadStore() (*CredentialStore, error) - (s).Get(serverURL) *Credentials // nil-receiver safe - (s).Set(serverURL, *Credentials) - (s).Delete(serverURL) - (s).Save() error - WipeCredentialsFile() error // file-level — replaces DeleteCredentials URL canonicalization is built in: trailing slash + surrounding whitespace are stripped before lookup/store, so http://x:7777 and http://x:7777/ hit the same bucket. Same rule cmd/pad/server_info.go was already applying via its now-redundant normalizeURL — removed. No top-level `default` field. The configured server (cfg.BaseURL() from ~/.pad/config.toml or --url) is always the source of truth for "which server am I targeting" — a separate `default` would create a second source of truth and the split-brain bugs that follow. ## Behavioral changes - `pad init --url <other>` against a server you've authed to before now reuses the saved credential instead of clobbering it. - `pad auth logout` removes only the configured server's entry. Other servers' tokens stay intact (pre-fix: wiped the whole file). - `pad auth whoami` reads only the entry matching the configured server. - Single-server users see no behavior change — one entry, identical shape per entry, identical UX. ## Compat shims removed LoadCredentials / SaveCredentials / DeleteCredentials are deleted outright (no // Deprecated lifecycle) — they're internal package helpers with no external API contract. All 10 call sites in cmd/pad/ and internal/cli/ are migrated to the per-server API in this PR. ## Tests internal/cli/credentials_test.go (15 tests): - File missing / empty → empty store (callers don't need nil checks) - v1 format reads + migrates in memory - v1 with empty token → empty store (no phantom entries) - v1 migration is durable on first Save (file flips to v2) - v2 round-trip preserves multiple entries - Set adds + replaces; mirrors URL into ServerURL field - Delete keeps siblings (multi-server keystone behavior) - Delete on absent key is a no-op - Nil receiver Get/Delete don't panic (NewClientFromURL relies on this) - URL normalization (trailing slash + whitespace) - Save preserves all entries across the file boundary - Save uses 0600 permissions - WipeCredentialsFile removes the file + is idempotent - Garbage file errors loudly (so we never silently lose data) Existing tests unchanged. Full suite + lint + web-check green. Closes: TASK-1228. Implements: IDEA-1226.	2026-05-07 20:23:13 -04:00
xarmian	ce0be1ed0a	fix(auth): TokenAuth falls through invalid Bearer on public API paths (BUG-1227) (#434) ... Prior behavior: TokenAuth middleware rejected any invalid/malformed Bearer with 401 before dispatch — even on paths in isPublicAPIPath (/api/v1/auth/*, /health, share links, public plan-limits). A stale credential in ~/.pad/credentials.json (typically left over after wiping a test DB) made every CLI invocation 401 on the very first CheckSession() call, INCLUDING the endpoints needed to recover (login, forgot-password). Users could only fix it by manually deleting their credentials file. The matching IP-change-revoked branch in the same file already had the right pattern (middleware_auth.go:114-117): when the path is public, fall through to the handler unauthenticated and let it decide. This patch mirrors that across the four invalid-Bearer branches: - Authorization header doesn't start with "Bearer " - padsess_* token doesn't validate (stale or wiped session) - pad_* token format wrong (length, prefix) - pad_* token doesn't match a live API token Extracted into a small rejectInvalidBearer helper so the policy is visible in one place. Protected endpoints continue to 401 — the regression guard in TestTokenAuth_ProtectedPath_StillRejectsInvalidBearer pins that. Pre-existing bug; not introduced by TASK-1216 / TASK-1217. The new bootstrap flows just made it more visible because anyone testing fresh- install scenarios is likely to wipe DBs and end up with stale creds. Tests in middleware_auth_public_paths_test.go cover: - /auth/session with stale padsess_* Bearer → 200 with public payload - /auth/session with malformed Authorization → 200 - /auth/session with garbage token format → 200 - /auth/session with non-matching pad_* token → 200 - /auth/login with stale Bearer + valid creds → 200 (the actual user- visible recovery scenario) - Protected /workspaces with invalid Bearer → still 401 (regression) Closes: BUG-1227. Related: IDEA-1226 (per-server credentials — proper design fix; this is the safety-net fix that complements it).	2026-05-07 20:01:34 -04:00
xarmian	51959532ad	feat(auth): browser-based pad auth setup via /setup#token deep link (TASK-1216) (#432) ... * feat(auth): browser-based pad auth setup via /setup#token deep link (TASK-1216) `pad auth setup` now hands the operator a deep link into the browser-based /setup form by default, replacing the in-terminal email/name/password prompts. The browser flow gives them password-manager support, HTML5 email validation, and the live strength meter at zero CLI cost — the mechanism (logs-token bootstrap, /setup route, /api/v1/auth/session) was already shipped by TASK-1167 / PLAN-1166 for the Unraid use case. This just unifies the local-CLI install path onto the same flow. New `internal/cli/bootstrap.go::RunBrowserBootstrap`: - Reads <DataDir>/.bootstrap-token and prints `<BrowserURL>/setup#token=<TOKEN>` with the token in the URL fragment (not query) — fragments are scrubbed from the address bar by /setup's onMount before paint, so the secret doesn't survive in browser history (TASK-1167 F10). - Polls /api/v1/auth/session every 2s; returns nil when setup_required: false. Internal 5-min timeout uses a separate timer (not context.WithTimeout) so caller-ctx cancellation surfaces as ctx.Err() instead of being misreported as the helper's own timeout. - Idempotent: returns early if setup is already done, without touching the token file. - Dispatches on session.setup_method — "logs_token" reads the token, "open" (PAD_BYPASS_SETUP_TOKEN=true) prints a bare /setup URL, "local_cli" / unknown returns an error directing the user to --cli-prompt. `pad auth setup` is rewired to call the helper, then chain doBrowserLogin so the user ends up authenticated on the CLI — preserving the post- condition of the legacy --cli-prompt path. Two browser approvals (admin creation, CLI auth) but each is one click in a browser the operator already has open. The legacy TTY path lives on behind --cli-prompt as a zero-cost hedge per IDEA-1179. Existing promptAndBootstrap / readPassword helpers are left in place — TASK-1217 will audit whether they can be removed once pad init is on the new flow too. Tests in internal/cli/bootstrap_test.go cover: idempotent session check, logs_token happy path, open mode, missing/empty token error paths, local_cli + unknown method rejection, internal timeout firing with the friendly message, caller-ctx cancellation propagating ctx.Err() (not timeout error). bootstrapPollInterval / bootstrapPollTimeout are vars so the timeout-branch test can run in 100ms instead of 5min. Implements: IDEA-1179 (auth-setup half). Out of scope: pad init integration → TASK-1217. Out of scope: post-/setup workspace dead-end → IDEA-1215. * docs(cli): clarify RunBrowserBootstrap caller staging across TASK-1216 / TASK-1217 Codex review (round 1) read the docstring and flagged that `pad init` isn't on the new helper. That wiring is TASK-1217's scope by design (one task = one PR per CONVE-2; TASK-1217 has a hard blocked-by link to TASK-1216). Tighten the docstring to make the staging explicit so a reader of the diff alone doesn't conclude it's a missing wire-up.	2026-05-07 17:28:29 -04:00
xarmian	40352a32e1	feat(auth): PAD_BYPASS_SETUP_TOKEN open-bootstrap escape hatch (#429) ... Adds an env-var that lets self-host operators on trusted networks (Unraid behind a firewall, Tailscale-only deployments, homelabs) claim the first admin via the web UI without copying a bootstrap token out of the container logs. Behavior when PAD_BYPASS_SETUP_TOKEN=true: - handleBootstrap accepts non-loopback first-admin POSTs without an X-Bootstrap-Token header. The UserCount==0 invariant is unchanged, so the bypass auto-closes the moment the first admin claims the seat (subsequent bootstrap requests get 409 regardless of bypass). - handleSessionCheck returns setup_method=open so the /setup page skips the paste-token UI and renders the form directly. - Token generation is skipped at startup (no .bootstrap-token file written). A distinct WARN-flavored banner makes the open-mode trade-off obvious in operator logs. - Cloud mode (PAD_CLOUD/PAD_MODE=cloud) ignores the flag entirely. Three layers of defense: cmd/pad masks the env-var with !cfg.IsCloudServer(), Server.openBootstrapEnabled() checks !s.cloudMode, and the cloud branch in handleBootstrap never reads the bypass field. Unraid template gets a new "Bypass Setup Token" field (default false, Display="always") with a description that calls out the trust-the- network trade-off. Tests pin all the security-critical contracts: bypass admits non- loopback, bypass off keeps existing 403, cloud mode hard-ignores, loopback works either way, post-bootstrap gate stays closed, bypass wins over logs_token in session payload, cloud mode never advertises 'open' setup method. Codex review: CLEAN (round 1).	2026-05-06 13:27:12 -04:00
xarmian	05a9665f50	feat(auth): first-run logs-token bootstrap flow (TASK-1167) (#424) ... One-time bootstrap token generated on first start with no users in self-host mode. Token is logged in a banner the operator can grab from `docker logs`, persists at <DataDir>/.bootstrap-token (mode 0600), and bypasses the loopback-only gate via the X-Bootstrap-Token header — letting the user claim the first admin from a remote browser at /setup#token=<x>. Header-only contract + URL-fragment (browser-only, never transmitted) + log-redaction middleware keeps the secret out of access logs, proxy logs, and browser history. Cloud mode unchanged: token never loaded, never honored. Validate → UserCount-check → CreateUser → consume sequence is mutex-serialized to prevent concurrent valid-token requests from creating multiple admins. Part of PLAN-1166 (Pad on Unraid — Community Apps launch).	2026-05-06 08:40:11 -04:00
xarmian	1ff6158468	feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) (#415) ... * feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) Foundation for PLAN-1100 (client-side permission audit). Lands the primitive that every other task in the plan consumes, with no UI behavior changes. Server: - new GET /api/v1/workspaces/{ws}/me — returns role, collection_access, visible_collection_ids (computed via VisibleCollectionIDs / GuestVisibleCollectionIDs so it covers system collections, member access, direct collection grants, and item-grant collections), plus the user's direct collection_grants and item_grants. - admins normalize to "owner"; legacy workspace-scoped tokens normalize to "editor"; non-members with no grants are rejected upstream by RequireWorkspaceAccess and never reach the handler. Frontend: - new $lib/utils/permissions module exporting pure cascade functions: canEditWorkspace / canViewCollection / canEditCollection / canViewItem / canEditItem. - cascade mirrors server's ResolveUserPermission exactly: owner → item grant → collection grant → membership role + visibility so item grant beats collection grant beats role even when less permissive (ItemGrant.view + CollectionGrant.edit on same item → effective view). - workspaceStore wraps the pure functions with currentMembership state fetched in setCurrent. New getters: currentRole, currentMembership, isOwner, canEditWorkspace; new methods: canViewCollection / canEditCollection / canViewItem / canEditItem. - WorkspaceMembership type added. - api.workspaces.me(slug) added. Refactor: - settings/+page.svelte, [collection]/+page.svelte, [collection]/[slug]/+page.svelte: drop open-coded role derivation (members.find + m.role open-codes), consume workspaceStore.isOwner. members.list calls remain — still needed for assignee dropdowns / member rows in settings — only the role-derivation path moves to the store. Tests: - server: handlers_me_test.go covers 6 scenarios (admin, editor with all-access, viewer with collection grant, restricted member, guest with item grant, non-member with no grants). - frontend unit tests deferred — web/ has no unit-test runner today. Pure-function module makes them trivial to add when the runner lands. Cascade is independently covered by store/permissions_test.go and store/grants_test.go on the server. Parent: PLAN-1100. * fix(workspace): per-item visibility uses strict full-access set + setCurrent race guard per Codex review (round 1) P1: canViewItem fell back to canViewCollection, which uses the broad nav set (visible_collection_ids — includes collections containing item-granted items so they appear in nav). This meant a guest with one ItemGrant on TASK-5 in Tasks would see canViewItem(any-other-task-in-Tasks) return true, while the server only allows direct item grants or full collection grants. Fix: /me now also returns full_access_collection_ids — the strict set of collections in which every item is accessible (collection grants + member_collection_access + system collections; item-grant collections intentionally excluded). This mirrors guestResourceFilter's fullCollIDs in handlers. canViewItem and canEditItem now consult full_access_collection_ids on the membership-fallthrough path, NOT the nav set. Test added: TestMe_GuestWithItemGrant now asserts the item-grant collection is in visible_collection_ids (nav) but NOT in full_access_collection_ids (strict). TestMe_RestrictedMember updated to check both sets. P2: workspaceStore.setCurrent had no guard against stale async /me responses. A slow /me for workspace A could clobber a freshly-fetched membership for workspace B if the user navigated mid-flight, briefly exposing permission-gated UI for the wrong workspace. Fix: monotonic membershipSeq counter incremented per setCurrent / create call. Each /me response only writes back if its captured token still matches at resolution time. Also clears currentMembership immediately on setCurrent so helpers don't briefly answer "yes" using the previous workspace's grants while /me is in flight. Parent: PLAN-1100. Refs TASK-1101 PR #415. * fix(workspace): canEditCollection uses strict full-access set per Codex review (round 2) Same nav-vs-strict bug pattern as round 1's canViewItem fix, but in canEditCollection. The editor-membership fallback path previously gated on canViewCollection (broad nav predicate using visible_collection_ids), which incorrectly returned true for a restricted editor whose only access to a collection was an item grant. The collection appears in nav (correct) but the editor must NOT see collection-wide write affordances like "+ New" because the server rejects collection-level writes there. Fix: editor membership fallback now requires either collection_access === "all" or the collection to be in full_access_collection_ids. canEditItem already used full_access_collection_ids on its fallback path (it was added in round 1) — verified unchanged. Parent: PLAN-1100. Refs TASK-1101 PR #415.	2026-05-05 08:52:05 -04:00
xarmian	89e9551ae3	test(store): end-to-end onboarding walkthroughs for scrum + product (TASK-1151) (#410) ... Mirrors TestOnboardingFlow_FullWalkthrough_Startup (PR #405) for the two newly-seeded software-category templates. Two new test fns, same three-phase shape: Phase 1 — Fresh seed: - Four onboarding seeds land at the right item_numbers + statuses. - Conventions + playbooks present (after the user-facing seeds). - Primary entry starts in its initial status (BACK-1=new for scrum; FEAT-1=proposed for product) — the gate the dashboard banner relies on. Phase 2 — Agent walks user through populating real items: - Primary's status flips out of initial (signaling engagement). - Real workspace activity gets captured in the template's verbs: scrum: a sprint, three real backlog items linked to it, one bug product: a roadmap commitment, three features under it, one user-feedback item from a sales call - Primary flips to terminal (BACK-1 → done; FEAT-1 → shipped) — banner hides on next dashboard refresh. Phase 3 — Idempotency on re-trigger: - User's items remain untouched. - Primary stays at terminal status — re-seed must NOT reset to initial, which would silently re-show the banner. - No duplicate seed items. - Conventions + playbooks counts unchanged. Reuses the helpers added in PR #405 (findItemByTitle, extractStatus, setItemStatus, countItemsInCollection) — no new helpers needed. This is the gate task for PLAN-1146. With this merged, scrum + product now have the same coverage as startup did after PLAN-1131. Parent: PLAN-1146.	2026-05-04 12:12:51 -04:00
xarmian	abf017c4e7	feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409) ... The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum workspaces surface "use pad to get BACK-1", product workspaces surface "use pad to get FEAT-1", and any future template that ships an agent-onboarding seed declares its primary ref once and gets the banner / hint for free. Mechanism: 1. WorkspaceTemplate gains an OnboardingPrimaryRef string field — the canonical declaration of "this template's IDEA-1-style primary entry." Set per template that ships the pattern (startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1"); left empty for hiring/interviewing/demo where the agent-onboarding pattern intentionally doesn't apply. 2. Server: handleGetDashboard identifies the seeded primary by walking allItems looking for item_number=1 + source="template" + created_by="system" + collection_slug ∈ {ideas, backlog, features}. The collection-slug whitelist is what keeps hiring's REQ-1 (also seeded with item_number=1 + source=template) from being flagged as an onboarding entry — those are example items, not agent scripts. The dashboard response gains an onboarding_seed field with ref/title/slug/collection_slug/status plus a server-computed `active` boolean (true iff status equals the schema initial value). 3. CLI: printOnboardingHints accepts the template name, looks up the primary ref via collections.GetTemplate, and prints the right "use pad to get X-1" line. Templates without a declared primary skip the line entirely (so hiring's pad init success doesn't promise a non-existent BACK-1 / IDEA-1). 4. Web frontend: dashboard reads dashboard.onboarding_seed, gates the banner on `active=true`, passes ref/slug/collection to OnboardingIdeaBanner. The component renders the trigger phrase, copy button, and "Read it first" deep link from those props — no more hardcoded IDEA-1. ensureWorkspace's signature gains a returned templateName so init.go + main.go can pass it through to printOnboardingHints. The five existing test call sites updated. New tests: internal/collections/templates_test.go - TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template OnboardingPrimaryRef values (and the explicit emptiness of hiring/interviewing/demo). internal/server/handlers_dashboard_test.go - TestDashboardOnboardingSeed_StartupTemplate - TestDashboardOnboardingSeed_ScrumTemplate - TestDashboardOnboardingSeed_ProductTemplate - TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed — hiring's REQ-1 is example data, not an onboarding entry) - TestDashboardOnboardingSeed_EmptyWorkspace (no template) Removes the loadIdeaOne race-guard from +page.svelte — the dashboard poll itself now carries the onboarding_seed.active flag so the banner state lives entirely in the dashboard response. Drops ~50 lines of frontend code. Parent: PLAN-1146.	2026-05-04 12:05:07 -04:00
xarmian	8fc0cb3b8b	feat(collections): seed onboarding items + explicit prefixes for scrum + product templates (TASK-1149) (#408) ... * feat(collections): seed onboarding items + add explicit prefixes for scrum + product templates (TASK-1149) Mirrors TASK-1133's pattern (PR #402) for the remaining software-category templates. After this lands: - fresh `pad workspace init --template scrum` → BACK-1 / SPRINT-2 / BUG-3 / DOC-4 - fresh `pad workspace init --template product` → FEAT-1 / FB-2 / ROAD-3 / DOC-4 Each is a first-person note from the workspace owner's future self — agent-invocable via `/pad let's discuss <REF>`, schema-aware terminal verbs ("mark me done" / "completed" / "shipped" / "archived"), no "tutorial" / "lesson" language. Bodies pulled verbatim from DOC-1152 (scrum) and DOC-1153 (product). Precondition fix: explicit Prefix set on five collections so DerivePrefix doesn't yield awkward refs: Backlog BACKL → BACK Sprints SPRIN → SPRINT Features FEATU → FEAT Feedback FEEDB → FB Roadmap Items RI → ROAD Mirrors hiring template's pattern of explicit prefixes on its custom collections. Existing scrum/product workspaces (forward-only fix) keep their derived prefixes — the seeder doesn't migrate. New tests: internal/collections/templates_test.go - TestScrumOnboardingItemsOrderAndShape - TestProductOnboardingItemsOrderAndShape - TestScrumProductTemplatesShipOnboardingSeedItems - TestScrumProductTemplatesUseExplicitFriendlyPrefixes (locks the prefix-fix precondition) internal/store/items_test.go - TestSeedCollectionsFromTemplateScrumRefSequence - TestSeedCollectionsFromTemplateProductRefSequence (Both also assert the prefix lands on each seeded item — drift in templates.go would surface here as a test failure pointing at the PLAN-1146 prefix precondition.) Existing onboarding test (TestSeedCollectionsFromTemplateStartupRefSequence) still passes — startup template untouched. Parent: PLAN-1146. Source content: DOC-1152, DOC-1153. * docs(comments): clarify the post-signup hint is wired in TASK-1150, not this PR (Codex review round 1) Codex flagged that the helper-file + templates.go comments said things like "the post-signup hint will name BACK-1" — which read as "it does today" but actually means "it will once TASK-1150 lands." Until that ships, the dashboard banner and CLI hint still hardcode IDEA-1 from PR #403, so a fresh scrum/product workspace gets the seeded items but no UI prompt that names them. Comments now explicitly call out the in-flight state so readers between this PR and TASK-1150 know what's wired and what isn't. No behavior change.	2026-05-04 11:41:29 -04:00
xarmian	de9c87622a	test(store): end-to-end onboarding walkthrough — fresh seed → user activity → idempotent re-trigger (TASK-1137) (#405) ... Validates the entire arc PLAN-1131 promises, at the store level (the API surface real workspace creation and real agent activity ultimately call through). The "agent" steps are stubbed via direct CRUD — the agent's reasoning is independently locked down by TASK-1136's resource test, so this layer focuses on the workspace state machine. Three phases mirror the user's experience: Phase 1 — Fresh-workspace seed: - The four onboarding seeds land at IDEA-1 / PLAN-2 / TASK-3 / DOC-4 in the right order with the right titles + statuses. - Conventions + playbooks land too (after the user-facing seeds). - IDEA-1 starts in status=new — the gate the post-signup hint relies on for "should I show the dashboard banner?". Phase 2 — Agent walks user through populating real items: - IDEA-1 status flips new → exploring (signaling engagement). - One real plan gets created, three tasks under it, one user-supplied idea — using the actual user-facing collections. - IDEA-1 status flips exploring → implemented (closes the loop; dashboard banner hides on next refresh). Phase 3 — Idempotency on re-trigger (server-startup auto-upgrade or explicit re-init): - User's plan / tasks / idea remain untouched. - IDEA-1 status STAYS at `implemented` — re-seeding must NOT reset it to `new`, which would silently re-show the banner and confuse the user. - No duplicate seed items. - Conventions + playbooks counts unchanged. Failure of any of these signals a regression in PLAN-1131's success criteria. Walk back through the design doc before "fixing" the test. Three small test helpers also added (findItemByTitle, extractStatus, setItemStatus, countItemsInCollection) — kept private to the package and used only by this test, but factored out so the assertions read cleanly. Parent: PLAN-1131. Origin: IDEA-1128.	2026-05-04 10:24:19 -04:00
xarmian	fc8ad67f0a	test(mcp): lock down IDEA-1 onboarding body verbatim across the resource pipeline (TASK-1136) (#404) ... The MCP resource pipeline `pad://workspace/{ws}/items/{ref}` already preserves arbitrary item content via formatItemAsMarkdown — covered by generic shape tests. This adds a targeted contract test for IDEA-1 specifically, since it's the seeded onboarding entry point that MCP clients (Claude Desktop, Cursor, Windsurf) hit when an agent is told "use pad to get IDEA-1". The test pulls the IDEA-1 body straight from collections.StartupOnboardingItems(), simulates the JSON envelope that `pad item show --format json` returns, runs it through readItem, and asserts: - The composed heading "# IDEA-1: <title>" precedes the body. - The full body content appears verbatim (substring match — layout flexibility preserved for future formatItemAsMarkdown tweaks). - Specific sections agents depend on are present: * "## What I'd find useful" (behavior contract) * "Then mark this idea implemented" (schema-valid terminal status, guards round-1 fix on PR #402 from silently regressing to "mark me done" which is invalid for the Ideas collection) * "## If I've already done this before" (idempotency contract) * `pad project dashboard` (code-fenced commands survive) - `- **status:** new` field row in metadata. - The dispatched CLI args use --format json (NOT --format markdown, which only emits the body and would fail other readers' contracts). No production code changes. The verification confirms the existing mechanism — same conclusion the task spec anticipated as the likely outcome ("may turn out to be a no-op"). Parent: PLAN-1131. Origin: IDEA-1128.	2026-05-04 10:17:11 -04:00
xarmian	96253f18a2	feat(collections): seed IDEA-1/PLAN-2/TASK-3/DOC-4 in startup workspaces (TASK-1133) (#402) ... * feat(collections): seed IDEA-1/PLAN-2/TASK-3/DOC-4 in startup workspaces (TASK-1133) A fresh `pad workspace init --template startup` now seeds four onboarding items — one per user-facing collection — that any agent can fetch and meaningfully converse around. The post-signup hint will name IDEA-1 specifically, but PLAN-2 / TASK-3 / DOC-4 are all viable entry points for `/pad let's discuss <REF>`. The bodies are first-person notes from the workspace owner's future self that introduce each collection's purpose by inviting a real conversation about the user's project — no marker, no skill detection, no schema fields. Word-audit clean: no "tutorial / lesson / step / walkthrough". Bodies pulled verbatim from DOC-1139. Sequence-stability: the existing seeder loop in store.SeedCollectionsFromTemplate already runs SeedItems before conventions/playbooks, so the workspace-scoped item_number sequence naturally lands at IDEA-1 / PLAN-2 / TASK-3 / DOC-4. A dedicated test (TestSeedCollectionsFromTemplateStartupRefSequence) locks the invariant down — drift means the post-signup hint silently misfires. Scope: startup template only. Scrum and product templates have different collection sets (Backlog/Sprints/Bugs and Features/Feedback/Roadmap respectively) and need their own bodies — tracked as follow-up under PLAN-1131. People-category templates (hiring, interviewing) are PLAN-1140. Parent: PLAN-1131. Source content: DOC-1139. * fix(collections): use schema-valid terminal statuses in onboarding bodies per Codex review (round 1) The seed bodies told agents to "mark me done" but ideas/plans/docs don't have a `done` terminal status — the HTTP/MCP update path validates select options, so an agent following the seeded copy would hit a validation error instead of completing the seed item. - IDEA-1: "mark this idea done" → "mark this idea implemented" (Ideas terminal: implemented|rejected) - PLAN-2: "mark me done" → "mark me completed" (Plans terminal: completed) - TASK-3: unchanged — "done" is the canonical terminal for Tasks - DOC-4: "mark me done" → "archive me" (Docs terminal: archived) Caught by Codex on PR #402. Same hard validation path the rest of the app honors — the seed copy needs to be schema-aware.	2026-05-04 09:10:31 -04:00
xarmian	40621ff58d	feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) (#400) ... * feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) Replaces the naive +1/-1 active-sessions accounting from TASK-961. The old logic bumped on JSON-RPC `initialize` and decremented on HTTP DELETE — but a client that crashed, lost network, or restarted mid-session never emitted DELETE, so the gauge drifted upward monotonically until the pad-cloud server restarted. Approach: - `internal/server/middleware_mcp_session.go` (new) — mcpSessionTracker is an in-memory map keyed by Mcp-Session-Id (the canonical header set by mcp-go's StreamableHTTPServer on initialize responses and echoed by the client on subsequent requests). Touch updates lastSeen on insert + refresh; evict removes; periodic sweep evicts entries older than the TTL. - Gauge is `Set(len(sessions))` via an onChange callback — single consistent observation per state-changing op, no risk of gauge drifting from map size on a multi-evict sweep. - Lifecycle: spawned by SetMCPTransport (alongside startMCPAuditWriter), shut down from Server.Stop. Idempotent on both sides. - Configurable via PAD_MCP_SESSION_TTL (default 30m) and PAD_MCP_SESSION_SWEEP_INTERVAL (default 5m). cmd/pad calls Server.SetMCPSessionTrackerConfig before SetMCPTransport. Other changes: - `recordMCPCallMetrics` no longer touches the active-sessions gauge. Updated comment + signature kept (callers pass the same args; the unused params are explicitly underscored). - `MCPAuditLog` middleware now calls trackMCPSession after next.ServeHTTP — single new line in the audit hot path. - `TestMCPAudit_BufferFull_DropsAndIncrementsCounter` updated to also shut down the new session tracker before bg.Wait(), since SetMCPTransport now spawns two goroutines on srv.bg. Test coverage (16 tests, all green under -race): - Tracker unit: touch insert/dedup, empty-id no-op, evict remove/non-existent, sweep eviction with single onChange, nil-onChange safety, concurrent touch/evict, run() clean shutdown. - Server-side integration: lifecycle happy path (initialize → call → DELETE leaves gauge at 0), failed initialize doesn't open, no-session-id no-op, nil tracker safety, idempotent start, DELETE evicts on any status (transient 5xx on shutdown still counts). - Regression guard: TestRecordMCPCallMetrics_DoesNotTouchSessionGauge pins that the audit-side helper has migrated off the gauge. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). Closes the "sessions drift on client crashes" caveat documented in the metric's help text + the Grafana panel description. * fix(metrics): emit Mcp-Session-Id + serialize gauge updates per Codex review (round 1) Two findings from Codex review on PR #400: 1. WithStateLess(true) wired StatelessSessionIdManager whose Generate() returns "" — mcp-go never set the Mcp-Session-Id response header in production, so the new tracker no-op'd on every initialize and the active-sessions gauge stayed at 0. Fix: introduce padMCPGenerateOnlySessionIDManager in cmd/pad/main.go. Generates a UUID per initialize (so the response carries the header — tracker can observe), but Validate accepts ANY incoming value (including empty / arbitrary). Preserves the original "stateless server, every request stands alone" contract while making the session-id observable. Documented why mcp-go's two shipped stateless managers don't fit (one breaks observability, the other breaks back-compat for clients that never echo the ID). 2. touch / evict / sweep computed `len(sessions)` under the mutex then released the lock BEFORE invoking onChange. Two concurrent inserts could compute (n=1, n=2) under the lock and then race the callback writes — last writer wins on the gauge, leaving it permanently inconsistent with the map size. Fix: hold the mutex across onChange. Trade-off documented: any future onChange that re-enters the tracker would deadlock, but that's a clear failure mode rather than silent metric corruption. Added TestMCPSessionTracker_OnChangeUnderLock that asserts a strictly-monotonic observation sequence under 32-goroutine concurrent inserts; passes 5x in a row under -race.	2026-05-03 17:40:18 -04:00
xarmian	1c409c8592	feat(metrics): emit mcp_authz_denials_total{reason=tier_mismatch} (TASK-1119) (#399) ... Wire the dispatcher-side scope-deny seam into the pad_mcp_authz_denials_total counter, completing the denial-reason vocabulary documented in TASK-961. internal/mcp/dispatch_http.go: - Add optional OnScopeDenied(method, urlPath) callback on HTTPHandlerDispatcher - Fire it from buildAuthedRequest right before returning the existing permission_denied error — same control flow, just observability added in front internal/server/middleware_auth.go: - Public Server.RecordMCPTierMismatch helper that bumps the counter. No MCP-origin context gate (unlike recordMCPAuthzDenial below) — the dispatcher is by construction MCP-only, so every invocation is inherently MCP-origin. cmd/pad/main.go: - Wire dispatcher.OnScopeDenied = srv.RecordMCPTierMismatch alongside the existing UserResolver / Lister fields. Safe to attach unconditionally — RecordMCPTierMismatch nil-checks metrics internally, mirroring the OAuth observer wiring pattern. Tests: - Three new dispatcher tests covering OnScopeDenied: fires once with the right (method, urlPath) on deny; does NOT fire on allow; nil hook is safe. - Server-side test for RecordMCPTierMismatch: counter increments, other denial reasons untouched, nil-metrics safe. Parent: PLAN-943. Follow-up to TASK-961 (PR #398).	2026-05-03 16:55:30 -04:00
xarmian	98c8b78d06	feat(metrics): MCP + OAuth observability metrics for /mcp (TASK-961) (#398) ... Plug MCP traffic and OAuth flow events into pad's existing internal/metrics Prometheus surface, plus a Grafana dashboard. Metrics (all under pad_*): - Counters: mcp_tool_calls_total{user_id,tool,status}, mcp_authz_denials_total{reason}, oauth_flows_total{stage}, oauth_token_revocations_total{reason} - Histograms: mcp_tool_call_duration_seconds{tool}, oauth_flow_duration_seconds{stage}, oauth_token_ttl_seconds - Gauges: mcp_active_sessions, oauth_active_tokens (callback collector) Wiring seams: MCPAuditLog (per-call), MCPBearerAuth (audience denials), emitMCPAuditDenied (rate-limit denials), RequireWorkspaceAccess (gated to MCP-origin via context — workspace_not_in_allowlist + not_a_member), OAuth handlers (per-stage flow events + per-handler latency), and internal/oauth/storage.go via a new SetRevocationObserver hook so the OAuth package stays metrics-naive. Cmd/pad wires both observers via Server.wireOAuthMetricsObserver(), called from both SetMetrics and SetOAuthServer for order-independence. Store helpers added (with full test coverage): - CountActiveOAuthAccessTokens — backs the active-tokens gauge - OldestAccessTokenIssuedAtByRequestID — backs the TTL observation Grafana dashboard at monitoring/grafana/mcp.json: 13 panels across MCP traffic + OAuth flow rows (rate-by-tool, p50/p95/p99 latency, status breakdown, denial reasons, active sessions, top-10 users, OAuth flow events by stage, OAuth handler p95, active tokens, revocations by reason, TTL p50/p95). Codex review caught one HIGH issue (round 1, fixed in same commit): the active-tokens collector originally emitted NewInvalidMetric on provider error, which propagates through Registry.Gather() and fails the entire /metrics scrape via promhttp's default error handler. Switched to log + skip-the-sample so a transient SQLite blip drops ONE gauge for one scrape rather than the whole observability surface. Added TestRegisterOAuthActiveTokensCollector_ErrorIsScrapeSafe to pin the contract. Tests cover increments, histogram bucket placement, callback collector freshness across mutations + error path, observer hook firing on user- initiated revocation + rotation + nil-safety, and per-helper unit tests for the server-side metric emission. Verified with `make check` (golangci-lint + go test ./... + web build).	2026-05-03 16:37:49 -04:00
xarmian	a1179f1c07	feat(dashboard): broaden agent-activity signal to include MCP source (TASK-1112) (#394) ... Renames the "has_cli_source" signal to "has_agent_activity" — semantically the dashboard flag for "this workspace's agent loop is wired up." Existing behavior is preserved (CLI activity still flips it on); the SQL widens to match source IN ('cli', 'mcp') so the signal stays correct if attribution is later split (today, all MCP-via-HTTPHandlerDispatcher activity persists as source='cli' per dispatch_http_test.go's contract). Renames: - store: WorkspaceHasCLISource → WorkspaceHasAgentActivity - dashboard struct: HasCLISource → HasAgentActivity - JSON tag: has_cli_source → has_agent_activity - Svelte state: hasCliSource → hasAgentActivity - Svelte fn: refreshHasCliSource → refreshHasAgentActivity - TS field: has_cli_source → has_agent_activity (DashboardData) - Test: TestWorkspaceHasCLISource* → TestWorkspaceHasAgentActivity* New test case in TestWorkspaceHasAgentActivity asserts that an item with source='mcp' also flips the signal on, exercising the broadened SQL clause. Comment updates explain today's "MCP attribution = source='cli'" reality so future readers don't search in vain for source='mcp' writers. The Svelte localStorage dismiss key (`pad-cli-banner-dismissed-`) is left unchanged in this PR — TASK-1114 will rename it with a soft-migration read of the old key for one release. This PR's goal is the rename + signal broadening, not the banner UX refactor. Unblocks TASK-1114 (banner two-mode refactor). Parent: PLAN-1111.	2026-05-03 10:50:54 -04:00
xarmian	92c05cb029	feat(auth): expose mcp_public_url on /auth/session (TASK-1113) (#393) ... Adds mcp_public_url to the /auth/session response (and the parallel setupStatePayload for the pre-bootstrap state). Sourced from the existing s.mcpPublicURL field that SetMCPTransport populates from PAD_MCP_PUBLIC_URL at startup. Empty string when unset — never null, never absent — so the web UI can branch on `mcp_public_url !== ''` as the gate for "this Pad instance exposes a Remote MCP server." Frontend gets a parallel `authStore.mcpPublicUrl` getter mirroring the existing `cloudMode` pattern. AuthSession.mcp_public_url is typed as required (string), since the server always emits it. Tests cover both shapes: empty string when PAD_MCP_PUBLIC_URL is unset (both pre-setup and post-bootstrap), and verbatim echo when configured. Unblocks TASK-1114 (banner two-mode refactor) which gates on this field. Parent: PLAN-1111. Note: AuthSession lives in web/src/lib/api/client.ts, not types/index.ts — the task description had the wrong file. Type was edited in client.ts.	2026-05-03 10:40:44 -04:00
xarmian	9b2234fce6	fix(workspaces): scope admin's personal workspace list to memberships (BUG-982) (#392) ... handleListWorkspaces special-cased server admins, routing them through an unfiltered store query that returned every non-deleted workspace regardless of membership. The admin's "switcher" therefore showed workspaces they had no member row in, labeled "shared with me" by the frontend even though they weren't actually shared. Filed in BUG-982 by the admin who saw the leak; the underlying mechanism would have leaked workspace metadata to any future server admin. The fix routes admins through the same GetUserWorkspaces path as every other authenticated user. Cross-tenant visibility for admins is still available via the admin-panel routes (/api/v1/admin/...), which call ListWorkspaces() directly with the appropriate auth gate — that's the correct surface for "see all workspaces on this server." Drive-by cleanups along the way: - Add ws.HydrateDerivedFields() to both branches of GetUserWorkspaces (member + guest) for parity with the admin path's previous behavior. Workspace context fields now hydrate consistently across all callers. - Delete the unused ListWorkspacesForUser store function. Its name implied per-user filtering, its body returned every workspace — pure footgun for any future code that grepped by name. Inline the no-userID branch into ListWorkspaces() (still used by the admin panel and pre-auth bootstrap). OUT OF SCOPE — handled by a follow-up Plan parented to PLAN-259 (Security Review): middleware_auth.go:449 still grants server admins implicit `owner` role on every workspace they navigate to. This PR closes the *listing* leak so admins no longer see workspaces in their switcher. It does NOT yet address the deeper concern in BUG-982's body — that on pad-cloud, admin access to other tenants should require an explicit auditable escalation flow (confirmation, audit log entry, owner notification, time-bound session, visible escalation banner). That's design-heavy and gets its own Plan. Tests: new internal/server/handlers_workspaces_test.go verifies that an admin who is NOT a member of a workspace does not see it in their listing, and that adding them as an explicit member restores visibility. Sister test confirms the existing non-admin behavior is unchanged. Both pass on SQLite and on Postgres via make test-pg. Full ./internal/server and ./internal/store suites stay green.	2026-05-03 00:35:31 -04:00
xarmian	6e4c7f617b	fix(timeline): drop \xff cursor sentinel that broke Postgres pagination (BUG-1086) (#391) ... The timeline handler defaulted the cursor's beforeID to the literal byte "\xff" as a sentinel intended to "sort after any UUID". SQLite tolerates that in TEXT columns, but Postgres rejects it as an invalid UTF-8 byte sequence (SQLSTATE 22021 — "invalid byte sequence for encoding 'UTF8': 0xff"), causing every timeline tab load on pad cloud to return 500. Reproduced empirically against the test Postgres with a one-line probe that issues a TEXT-typed bind of "\xff" — same error string the bug reported. The fix removes the sentinel and distinguishes three cursor cases in the handler: 1. Neither `before` nor `before_id` (true first page) → store gets beforeID = "" and drops the id tie-breaker from the WHERE clause entirely. Just `WHERE created_at < ?`. 2. Both supplied (normal cursor pagination) → unchanged. 3. `before` supplied without `before_id` (anomalous but possible for external clients) → use "g" as a UUID-safe upper-bound sentinel. Lowercase-hex UUIDs are bounded by "f", so "g" sorts above them in every reasonable collation while remaining valid UTF-8. This preserves the legacy semantics of including same-second rows that the naive `created_at < ?` would drop — a regression Codex caught on round 1 of review. The id-predicate branching is applied symmetrically across all three *BeforeTime store functions: ListCommentsBeforeTime, ListDocumentActivityBeforeTime, ListItemVersionsBeforeTime. New test file internal/store/timeline_pagination_test.go covers: - No-cursor first-page path (the broken one) - Real (timestamp, id) cursor pagination - Same-second cursor with sentinel id (regression guard) - Limit respected - Activity and version BeforeTime no-cursor paths All six pass on SQLite and on real Postgres via `make test-pg`. Full ./internal/store and ./internal/server suites stay green on Postgres.	2026-05-03 00:12:50 -04:00
xarmian	f9d3244660	feat(connected-apps): user-facing OAuth connection management page (TASK-954) (#390) ... * feat(connected-apps): user-facing OAuth connection management page (TASK-954) Adds /console/connected-apps where a logged-in user can see every OAuth grant chain they've authorized via the MCP consent flow (Claude Desktop, Cursor, …) and revoke any of them. Joins to the DCR client metadata for the display name + logo, and to the MCP audit log (TASK-960) for the "last used" + "30-day calls" columns. Pieces: - internal/store/connected_apps.go — ListUserOAuthConnections walks oauth_access_tokens + oauth_refresh_tokens, dedups by request_id, hydrates client metadata, parses session_data for the workspace allow-list, classifies granted_scopes into a coarse capability tier. RevokeUserOAuthConnection verifies ownership (ErrConnection NotFound for stranger's chains — anti-enumeration; same shape as for unknown chains) then calls the existing RevokeRefreshTokenFamily + RevokeAccessTokenFamily so the next /mcp call gets 401. - internal/models/connected_apps.go — OAuthConnection + CapabilityTier models. - internal/server/handlers_connected_apps.go — REST endpoints: GET /api/v1/connected-apps (list) + DELETE /api/v1/connected-apps/{id} (revoke, idempotent, 204). Wrapped in requireCloudMode group. List enriches with MCPConnectionStatsForUser (audit aggregates) — soft-fails on the audit lookup so a broken audit table degrades to "no last-used data" instead of a broken page. Revoke records an "oauth_connection_revoked" entry in audit_trail via the existing CreateActivity path. - web/src/routes/console/connected-apps/+page.svelte — list with per-app card (logo, name, capability badge, workspace chips with +N expander, connected/last-used relative times, 30-day count), Details expander showing scope_string + workspace list + redirect URIs, Revoke button → confirm modal → optimistic refresh, friendly empty state linking to /connect. - web/src/routes/console/+layout.svelte — Connected Apps nav link (cloud-mode-gated, between Settings and Billing). - web/src/lib/api/client.ts + types/index.ts — typed client + ConnectedApp interface. Tests cover: - Store: chain dedup across rotation siblings, subject filtering (Bob can't see Alice's), inactive chains excluded, ownership check on revoke, idempotent re-revoke, capability tier mapping, session-data allowed_workspaces parsing (both []string and JSON []interface{} round-trips). - Handler: cloud-mode gate (404 outside), owner-only filtering, DTO field shape + audit enrichment populating last_used_at + calls_30d, revoke ownership 404 (not 403 — anti-enumeration), idempotent 204, audit_trail row written. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(connected-apps): point empty-state link at getpad.dev (Codex review round 1) Codex caught: the empty-state link to /connect 404s because /connect is a pad-web (marketing site) route, not a docapp route. From inside the authenticated console at app.getpad.dev, the right target is the absolute https://getpad.dev/connect URL — same pattern the +error.svelte page uses for its "Back to getpad.dev" + "/docs" links. * fix(console nav): exclude /console/connected-apps from Workspaces active match (Codex round 2) Codex caught: the Workspaces nav predicate `isActive('/console') && !isActive('/console/settings') && ...` was missing the new /console/connected-apps prefix, so both Workspaces AND Connected Apps lit up when viewing the connected-apps page. Same shape as the existing exclusions for settings / billing / admin.	2026-05-02 23:21:07 -04:00
xarmian	d8b1d98e08	feat(mcp): persistent audit log for /mcp tool calls (TASK-960) (#389) ... * feat(mcp): persistent audit log for /mcp tool calls (TASK-960) Adds a 90-day-retention audit log of every MCP request. Drives the "last used" + "30-day calls" columns the connected-apps page (TASK-954) will read, and gives ops + on-call a forensics surface via a new admin /console/admin/mcp-audit page. Schema deviation from the spec, documented in migration 049: the original task body called for `token_id REFERENCES oauth_tokens(id)` but pad has no `oauth_tokens` table — instead an OAuth grant chain is identified by `request_id` (preserved across refresh-token rotations, see migration 048), and PAT-authenticated MCP requests have no OAuth identity at all. The audit row therefore carries `(token_kind, token_ref)` — `oauth` + request_id for OAuth, or `pat` + api_tokens.id for PATs. The connected-apps page in TASK-954 will filter on token_kind='oauth' to surface third-party connections only. Pieces: - internal/store/migrations/049_mcp_audit.sql + pgmigrations/028 — table. - internal/models/mcp_audit.go — typed entry + 30-day stats DTO. - internal/store/mcp_audit.go — insert / list-by-user / list-by-connection / list-all / per-connection-stats aggregator / 90-day retention sweeper. - internal/server/middleware_mcp_audit.go — async writer + sweeper + middleware that wraps /mcp behind MCPBearerAuth. Hot path is non-blocking enqueue with drop-on-overflow + atomic drop counter. - internal/server/middleware_mcp_auth.go — both PAT + OAuth branches now stash WithMCPTokenIdentity so the audit row attributes correctly. - internal/server/handlers_mcp_audit.go — read endpoints: GET /api/v1/connected-apps/{id}/audit (owner-scoped) + GET /api/v1/admin/mcp-audit (admin-only). - web/src/routes/console/admin/mcp-audit/+page.svelte + tab in admin layout. - Tests cover required-field validation, round-trip, pagination, owner-only filtering, last-used + 30-day aggregates, retention sweep, body-sniff parser, canonical-JSON arg hashing, buffer-full drop path, status-to-result classification, admin gate, DTO field shape. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(mcp-audit): emit denied row on rate-limit reject per Codex review (round 1) PR #389 round 1 caught: MCPAuditLog is mounted INSIDE MCPBearerAuth, so when bearer auth's per-token rate-limit fires (429) it returns before next.ServeHTTP — and the wrapping audit middleware never sees the response. classifyMCPResult mapped 401/403/429 with no path that could actually reach it. Fix: emitMCPAuditDenied helper called directly from the rate-limit deny branches of both PAT + OAuth paths. Resolved user + token identity are already in scope at that point, so the audit row gets attributed correctly. Pre-auth rejections (no/invalid bearer) stay un-audited because there's no user to attribute them to and the audit_trail table covers those auth-event signals already. Threading: handleMCPPATAuth + handleMCPOAuthAuth now take the entry timestamp so the denied row carries real latency. Test: TestMCPAudit_RateLimited_RecordsDeniedRow drives a real PAT through the rate limiter, drains to 429, and asserts the audit row lands with status="denied" + error_kind="rate_limited" + the right tool_name from the request body.	2026-05-02 22:56:10 -04:00
xarmian	42f6ce96e1	fix(mcp): normalize error envelope shape + extend code taxonomy + actionable hints (TASK-1077/1078/1079) (#388) ... Three independent improvements bundled as one PR because they all touch the same dispatcher error-emission surface; landing them piecemeal would churn the same lines repeatedly. ## TASK-1077 — uniform envelope shape Pre-fix some dispatchers emitted plain-string errors via `mcp.NewToolResultErrorf("%s: %s failed: %s", ...)`. Same underlying 404 surfaced in three different shapes across the surface (item lookup → structured envelope; note/decide → "item note: prefetch: 404 ..."; bulk-update per-row → bare error string). Inconsistent shape made it hard for agents to reason about errors uniformly. Three new helpers in errors.go: - validationFailedResult(cmdKey, msg, fixHint) — replaces the "X is required" / "invalid Y" chain across every dispatcher. - dispatcherErrorResult(cmdKey, op, err) — replaces the internal "build request: %s" / "encode body: %s" / "parse current: %s" chain. Always emits ErrServerError with a programmer-readable Hint. - upstreamHTTPErrorResult(...) — wraps every in-handler prefetch / sub-call HTTP failure through classifyHTTPStatusKind so the shape matches the main pipeline's responses exactly. Every NewToolResultErrorf call site in internal/mcp/dispatch_http*.go + catalog.go retrofitted. bulk-update's per-row `Error string` field flipped to `Error *ErrorPayload` so every row failure carries the same {code, message, hint} shape as a top-level failure. ## TASK-1078 — resource-kind-aware error codes Pre-fix every 4xx 404 collapsed to ErrItemNotFound regardless of what was being read; pad_workspace list returning 404 (route missing) reported `code: "item_not_found"` despite the call having nothing to do with items. Pre-fix every 5xx collapsed to ErrServerError, indistinguishable from dispatcher internal failures. Three new codes in errors.go: - ErrNotFound — resource-shaped 404s that AREN'T item lookups (collection, listing endpoint, link target, attachment). - ErrUpstreamError — 5xx with a structured body (transient backend failure). Distinct from ErrServerError (catch-all for dispatcher internal + un-mapped 4xx). - ErrBackendUnreachable — reserved for transport-level failures (DNS / connection refused / 5xx with no body); not yet emitted by classifyHTTPStatus but available for future transport-aware classification. - ErrWorkspaceRequired — reserved for the multi-workspace-token "ambiguous default" case (TASK-1076's deferred sister error; constant available even though dispatcher doesn't emit it yet). New ResourceKind enum (item/workspace/collection/listing/link/ attachment/unknown) lets callers tell the classifier what they were reading. classifyHTTPStatusKind is the new entry point; classifyHTTPStatus preserved as a legacy adapter for callers that haven't been retrofitted (pass ResourceUnknown → falls back to pre-TASK-1078 behaviour). Every retrofit call site passes its known kind + ref/slug, so 404s now route through the right code with a contextual message ("Item TASK-7 not found.", "Workspace foo not visible.", "Collection tasks not found.", etc.). ## TASK-1079 — actionable hints Pre-fix `hint` was usually `"404 page not found"` (chi's default NotFound body verbatim) or the upstream JSON envelope re-stringified. Either way: zero diagnostic value, sometimes outright misleading (double-stringified JSON in a hint field is hostile). Per-code hint generators in errors.go: - itemMissingHint — names the ref + route + suggests pad_item search / list as recovery. - workspaceMissingHint — names the slug + route + composes with the existing available_workspaces enrichment. - notFoundHintFor — kind-aware: collection 404 → "use pad_collection list to enumerate"; listing 404 → "verify the route matches the server's API surface (build version may be stale)"; etc. - authHintFor / permissionHintFor — point at re-auth / scope check. - upstreamHintFor — flags 5xx as "usually transient — retry once or check pad logs." extractUpstreamMessage parses pad's own structured `{error:{message}}` envelope when the upstream backend returned one, so hints lift the inner human-readable message out instead of dumping the literal JSON. Falls back to the raw body when the JSON shape doesn't match (no parse failure noise). ## Tests - TestDispatcher_AllErrorsUseStructuredEnvelope walks every special-case + link dispatcher's missing-required-input error path; pins the shape (code, message, hint all set; hint never just "404 page not found"). Adding a new dispatcher that uses NewToolResultErrorf will fail this test — it's the regression gate the DOD wants. - TestClassifyHTTPStatus_KindAware pins each ResourceKind → expected ErrorCode mapping for 404s. - TestClassifyHTTPStatus_HintsAreActionable pins that hints reference the actual route + ref + recovery tools, AND forbids the bare "404 page not found" passthrough that triggered Bug 17. - TestExtractUpstreamMessage covers the 7 input shapes the helper can see (structured envelope, empty inner, missing inner field, unparseable, wrong shape, empty, with extra fields). - Two existing tests updated to reflect the new shapes: TestClassifyHTTPStatus 5xx cases now expect ErrUpstreamError; TestMakeFanOutHandler_UnknownAction + TestActionEnv_Dispatch_ UnknownCmdPath substring searches updated for JSON-encoded quotes. ## Behavior diff agents will observe Same underlying 404, three example error envelopes: pad_item show TASK-MISSING: code: "item_not_found" message: "Item not found." hint: "Item \"TASK-MISSING\" not found. Route: /api/v1/.../items/TASK-MISSING. Try `pad_item search` or `pad_item list` to find the right ref." pad_workspace list (route 404): code: "unknown_workspace" message: "Workspace not visible to this session." hint: "Route: /api/v1/workspaces. Available workspaces: docapp, pad-web." pad_project dashboard (workspace doesn't exist): code: "unknown_workspace" message: "Workspace \"missing\" is not visible to this session." hint: "Workspace \"missing\" not visible. Route: /api/v1/workspaces/missing/dashboard. Available workspaces: docapp." Backend 500: code: "upstream_error" message: "pad item show failed: backend returned 500" hint: "Backend returned 500. Usually transient — retry once or check pad logs for the underlying error. Route: ..."	2026-05-02 22:10:12 -04:00
xarmian	22f6342794	fix(mcp): bundle BUG-1081 + BUG-1082 + TASK-1076 — three small MCP-UX fixes from dogfooding (#387) ... All three caught in Claude Desktop's second-round review against the deployed cloud build. Independent file surfaces, but they all polish the same MCP-tool-call user experience so they ride together. ## BUG-1081: star/unstar return structured JSON instead of 204 internal/server/handlers_stars.go — `handleStarItem` and `handleUnstarItem` previously returned 204 No Content. RESTfully fine, but the MCP HTTPHandlerDispatcher passes through whatever the handler wrote — empty body + 204 → empty MCP tool result. Agents had no signal whether the operation landed. BUG-989's earlier fix touched the CLI's text output via the JSON branch but missed the API endpoint itself. Fix: both endpoints now return 200 OK with `{ref, starred: bool}`. Mirrors the shape Claude's review requested + the broader "return enough info to be the next source of truth" pattern note/decide adopted. New test pins the wire shape including content-type. Negative control verified — reverting the handler fails the test with "expected 200, got 204" on the first assertion. ## BUG-1082: suggested_next surfaces orphans, not just plan-children internal/server/handlers_dashboard.go — the candidate loop only walked items that are children of an active plan. Workspaces without active plans (or with in-progress / high-priority items outside their active plans) got an empty suggested_next, even when the obvious answer was "continue your one in-progress task." BUG-990's earlier fix added in-progress to the active-plan scope but kept the orphan branch in scope-creep territory. Real dogfooding showed it's the common case for new workspaces. Fix: add a second pass that scans all items for in-progress (any priority — continuation always beats priority) and high/critical- priority open items not already in the active-plan candidates. Orphans rank lower than plan-children so existing plan-driven behavior is preserved when both are present. Reason text drops the plan-name reference for orphans. Two existing tests pinned the OLD "no suggestions when no active plans" behavior — that was pinning the bug. Updated to the new correct behavior. Added two new tests pinning the in-progress- beats-priority gating and the rank-below-active-plan ordering. ## TASK-1076: workspace auto-default from OAuth allow-list internal/mcp/dispatch_http.go + internal/mcp/dispatch_http_advanced.go — the dispatcher's preprocess flow now calls `maybeInjectWorkspace` after the existing --assign / --role resolution. When: - input["workspace"] is set → caller wins (no override) - d.Lister is nil → no-op (tests + non-OAuth paths) - lister returns 1 workspace → inject input["workspace"] = slug - lister returns 0 or N → no-op (caller must pass explicitly; route mapper's existing "missing required input" error surfaces if the route needs workspace) The lister already encodes the right policy (PAT auth → all the user's workspaces; wildcard token → same; specific allow-list → intersection with memberships) so we reuse it instead of building a parallel resolver. Auto-defaulting only when the resolved set collapses to one is the unambiguous case; multi-workspace tokens still require explicit choice (silently picking one would be a real audience-confusion hazard for write operations). Caller-passed workspace ALWAYS wins — agents that pass an explicit slug never see it silently overridden by the default. Lister error falls through to no-op (don't poison input on transient store hiccup). Tests pin all four matrix cases from the task spec plus three operational corner cases (nil lister, lister error, copy-on-write non-mutation). ## Combined CI surface `make check` clean across lint + tests + web. The test surface gained: - TestStarUnstar_ReturnsStructuredJSON (server) - TestDashboardSuggestedNextOrphan_InProgressBeatsPriority (server) - TestDashboardSuggestedNextOrphan_RanksBelowActivePlan (server) - TestMaybeInjectWorkspace_* (mcp, 7 cases) - TestDashboardSuggestedNextNoPlans + TestDashboardSuggestedNextFromPlannedPlan reframed from pin-the-old-bug to pin-the-new-correct-behavior	2026-05-02 21:31:48 -04:00
xarmian	40f636e6b2	fix(mcp): strip inherited chi.RouteCtxKey before synthesizing dispatched requests (TASK-1075) (#384) ... Every production /mcp tool call was returning 404 from the dispatcher's synthesized /api/v1/... request, surfacing in Claude Desktop / Cursor / ChatGPT as the generic \"{code:'item_not_found', hint:'404 page not found'}\" envelope on pad_workspace_list, pad_item_show, pad_project_dashboard, and every other tool. Codex review caught the actual cause. ## Root cause chi's Mux.ServeHTTP short-circuits when the inbound request context already carries a chi.RouteCtxKey (chi/v5/mux.go:71-75): rctx, _ := r.Context().Value(RouteCtxKey).(*Context) if rctx != nil { mx.handler.ServeHTTP(w, r) // bypass fresh routing return } That's the right behavior for chi's own Sub() / Mount() patterns (running as a sub-router under a parent), but wrong for our case: we synthesize a brand-new HTTP request that needs to route from scratch against the ROOT mux. In production every MCP call enters via chi's /mcp route — chi attaches a RouteCtxKey to the inbound request context, mcp-go threads that context through to the tool handler, and the dispatcher inherits it via http.NewRequestWithContext(ctx, ...). The synthesized /api/v1/workspaces request then runs through srv.ServeHTTP carrying the stale RouteCtxKey from /mcp — chi takes the short-circuit branch, skips its rctx.Reset() + RoutePath = \"/api/v1/...\" setup, the route table lookup runs against contaminated routing state, and the request falls through to chi's default NotFound handler. The body of that handler is the literal \"404 page not found\\n\" the user reported. ## Why tests passed pre-fix Existing dispatcher tests called Dispatch with context.Background() — no chi RouteCtxKey to inherit, no contamination. The bug was specific to the production path where requests enter via the chi- mounted /mcp endpoint. ## Fix In buildHTTPRequest (the central path EVERY synthesized request flows through — main writes, RMW prefetches, bulk-update PATCHes, link-create POSTs), shadow chi.RouteCtxKey with a typed nil before constructing the new request: ctx = context.WithValue(ctx, chi.RouteCtxKey, (*chi.Context)(nil)) chi's Value(RouteCtxKey).(*Context) on a typed-nil returns (nil, false), the `rctx != nil` check fails, and chi takes the fresh-routing branch as intended. We deliberately do NOT strip pad's own context values (WithCurrentUser, WithAPITokenAuth, TokenScopes, TokenAllowedWorkspaces) — those carry the authenticated user identity and OAuth scope/allow-list state the synthesized request needs. Only the chi-specific routing key is stripped. ## Tests Two added (both fail without the fix, pass with it — verified via git stash negative-control): - TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath: full integration shape — chi router with /mcp route whose handler invokes the dispatcher, which synthesizes a /api/v1/workspaces request that MUST reach the workspace handler. Pre-fix returns 405 Method Not Allowed (chi remembers /mcp's registered methods). Post-fix returns 200 with the workspace data round-tripped. - TestBuildHTTPRequest_StripsChiRouteCtx: unit-level pin on the strip itself — feeds buildHTTPRequest a context carrying a non-nil chi RouteCtx, asserts the resulting request's context type-asserts to nil at chi.RouteCtxKey. The integration test also pins (\"test setup\") that the inbound context DOES carry a RouteCtxKey under chi v5.2.5 — if chi ever changes that semantic the test fails loudly rather than silently passing for the wrong reason. ## Credit Found by Codex under /codex ask after my own initial trailing-slash hypothesis was empirically disproved.	2026-05-02 20:03:53 -04:00
xarmian	7429de3933	fix(oauth): CSP nonce on consent screen so the inline UI-state script can run (#383) ... Pasting the bare https://mcp.getpad.dev URL into Claude Desktop now reaches pad's consent screen, but the Allow button stays disabled even when the user picks workspaces. Cause: the consent template ships UI-state JS in an inline <script> block (workspace selection flips disabled=false on the Allow button + handles the wildcard mutual-exclusion warning), but pad's strict response CSP is "script-src 'self'" with no 'unsafe-inline' and no nonce — so the browser silently blocks the inline script and the Allow button stays at its initial disabled=true. Adopts the same nonce + strict-dynamic CSP pattern pad already uses for the SvelteKit SPA bootstrap (see server.go's setupRouter SPA route): renderConsent generates a per-request nonce via generateCSPNonce, sets a CSP header that authorizes that nonce ("script-src 'self' 'nonce-X' 'strict-dynamic'") before writing the body, and threads the same nonce into the template's <script> tag's nonce attribute. This is per-handler (overrides SecurityHeaders middleware on the consent response only); every other endpoint keeps the strict no-nonce baseline. matches the existing SPA-bootstrap nonce path exactly so future security-hardening on either side stays self-consistent. Adds TestOAuth_ConsentScreen_NonceCSPLetsInlineScriptRun pinning two facts: 1. CSP on the consent response carries a 'nonce-...' token in script-src (proves the override fired and we didn't fall back to the strict baseline). 2. The body's <script> tag carries the SAME nonce value (proves the two are linked — drift would re-introduce the bug). Both are necessary; either failing causes the browser to block.	2026-05-02 19:21:25 -04:00
xarmian	229d47e189	fix(oauth): treat empty-path/root trailing slash as equivalent (RFC 3986 §6.2.3) (#382) ... Real OAuth clients reconstruct the resource indicator from the URL the user pasted. URL parsing canonicalizes empty path → "/", so a client given "https://mcp.getpad.dev" emits "resource=https://mcp.getpad.dev/" — with a trailing slash that pad's canonical "https://mcp.getpad.dev" doesn't have. The strict string compare in audienceMatchingStrategy (and the matching audienceContains check on the RS side at /mcp) rejected these as distinct audiences and the connector flow died on "Requested audience https://mcp.getpad.dev/ is not the canonical audience https://mcp.getpad.dev." Per RFC 3986 §6.2.3 (Scheme-Based Normalization) those forms ARE equivalent for the HTTP scheme. Adds NormalizeAudience(s) and applies it on both sides of every audience comparison: - internal/oauth/audience.go: audienceMatchingStrategy normalizes the canonical, then checks each needle and the haystack against it via audienceListContainsNormalized. - internal/server/middleware_mcp_auth.go: audienceContains (the RS-side gate at /mcp) normalizes both sides too. Mirroring the rule keeps AS and RS in lockstep — without it, tokens the AS minted for a slashed audience would fail validation at /mcp. Per Codex review #386 round 1, normalization is restricted to URIs whose path component is exactly the root ("/"). Earlier draft trimmed ANY trailing "/", which would have made "https://host/mcp" and "https://host/mcp/" compare equal — distinct HTTP resources collapsing to one audience is a real audience-confusion attack surface. The boundary is enforced via url.Parse: only normalize when u.Host is non-empty AND u.Path == "/" AND there's no query/fragment. Anything else returns byte-exact. TestNormalizeAudience pins both branches (root case trims; non-root paths, hostless strings, queries, fragments, and unparseable inputs all stay as-is). TestAudienceStrategy_PathSlashIsNotEquivalent guards the strategy layer directly: even with normalization active, "/mcp" and "/mcp/" are kept distinct.	2026-05-02 19:01:56 -04:00
xarmian	ba303e456f	fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381) ... Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically.	2026-05-02 18:36:55 -04:00
xarmian	69e471db8f	fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) (#380) ... * fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) Real MCP clients (Claude Desktop, Cursor as of 2026-05) don't send the RFC 8707 `resource` parameter on /oauth/authorize at all. Before this fix, translateResourceToAudience only translated resource→audience when resource= was present, so empty-resource requests reached fosite's audienceMatchingStrategy with an empty needle and got rejected with "resource parameter is required (RFC 8707)". fosite then redirected to the client's redirect_uri with ?error=invalid_request&error_description=..., and Claude's backend callback failed with the pydantic envelope "code: Field required" (because no `code` parameter was in the redirect query). RFC 8707 §2 marks the resource parameter OPTIONAL; servers with a single canonical audience are expected to default to it. pad's OAuth server has exactly one canonical audience by construction (cfg.MCPPublicURL + "/mcp"), so the right policy is to inject canonical when the client sends neither resource= nor audience=. Now translateResourceToAudience handles three cases in priority order: 1. audience= already set — leave both keys untouched. 2. resource= present — copy to audience= (existing path). 3. Neither present — inject canonical into both. The token gets bound to canonical exactly as if the client had sent it. audienceMatchingStrategy's strict empty-needle reject stays as defense in depth: case 3 only fires when canonical is configured (main.go won't construct the OAuth server otherwise), but if some future code path bypasses the translation helper, the matching strategy still fails loudly rather than minting an unbound token. Adds TestOAuth_Authorize_AcceptsNoResource_DefaultsToCanonical pinning Claude Desktop's exact request shape (no resource=, no audience=). Pairs with the existing AcceptsResourceOnly and audience-mismatch tests to lock in the full /authorize matrix. * docs(oauth): document RFC 8707 cross-server replay trade-off + audit log Per Codex review #383 round 1: defaulting to canonical when the client omits resource= weakens the cross-server replay defense RFC 8707 was designed to provide. Threat is the confused-deputy attack — malicious MCP server lies that pad's AS is its AS, client (which doesn't send resource=) drives a flow against pad's AS, pad mints a token bound to canonical, client returns it to the attacker, attacker replays at pad's /mcp. We're shipping with the default-to-canonical path because every real-world MCP client (Claude Desktop / Cursor / ChatGPT as of 2026-05) omits resource= and the alternative is "remote MCP doesn't work for any client until the entire ecosystem adopts RFC 8707." Mitigations now documented in the comment + active in the code: - Consent screen (TASK-952) is the trust anchor. Every grant requires a click-through that identifies the resource as "your Pad workspaces" and lists the user's actual workspace names. A user attempting to connect to a non-pad MCP server who lands on pad's consent screen sees the mismatch. - Matches industry practice (GitHub / Google / Atlassian all rely on consent-as-trust-anchor since RFC 8707 is barely deployed). - audienceMatchingStrategy's strict empty-needle reject stays as defense in depth — fires when canonical is unset and on any future code path that bypasses the helper. - Audit log (slog.Warn) on every default-fire gives ops a signal to detect anomalies — a spike of defaulted requests from a previously-unseen client_id is the earliest detectable shape of a confused-deputy attempt. Future task tracks restoring the strict reject once Claude / Cursor / ChatGPT all send resource=.	2026-05-02 16:50:34 -04:00
xarmian	9eb1a35f16	feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) (#379) ... * feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) Closes the last open work item in PLAN-943. HTTPHandlerDispatcher's unknown_workspace error envelope now populates available_workspaces filtered by the OAuth token's consent allow-list (TASK-952), so an agent never sees workspace slugs the user didn't explicitly grant. ## What changed - `HTTPHandlerDispatcher` gains a `Lister WorkspaceLister` field. Production wires `mcpserver.NewOAuthWorkspaceLister(s)`; tests can supply mocks. - `packageHTTPResponse` now takes a `lister` parameter and threads it down to `classifyHTTPStatus`. Both call sites in the package updated. - New `oauthWorkspaceLister` reads three things from request context: - `server.CurrentUserFromContext` — the requesting user. - `server.TokenAllowedWorkspacesFromContext` — the consent allow-list (TASK-953 plumbing). - `s.GetUserWorkspaces(user.ID)` — the user's full set. Returns the intersection. Wildcard (`["*"]`) and nil (PAT auth) short-circuit to "no filter" — the user's full set is returned in those cases since the token doesn't constrain workspaces. - `cmd/pad/main.go` wires the production lister. ## Privacy invariant A token whose allow-list is `[alpha, beta]` MUST NOT see "gamma" in the available_workspaces hint, even if the user is a member of gamma. Tested explicitly via TestUnknownWorkspace_AvailableWorkspaces_FilteredByAllowList — the test fakes a 4-workspace user membership, sets allow-list to 2, and asserts exactly 2 slugs appear in the filtered envelope. Without this filter, an attacker controlling an OAuth client could hit any random workspace slug, get the unknown_workspace envelope, and read OFF the user's full workspace list — defeating the whole point of the consent UI's per-workspace selection. ## Tests (18 new) 8 envelope round-trip tests pin every documented HTTP status → ErrorCode mapping (401 → auth_required, 403 → permission_denied, 404 generic → item_not_found, 404 workspace → unknown_workspace, 409 → conflict, 400/422 → validation_failed, 5xx → server_error, 418 → server_error fallback). 5 privacy-filter tests cover the allow-list shapes: specific-list-filters, wildcard-no-filter, no-allow-list-no-filter, no-user-empty-hints, store-error-empty-hints. 4 buildAllowSet unit tests for the helper. 1 end-to-end test through packageHTTPResponse. * fix(mcp): use req.Context() when packaging HTTP response (Codex round 1) Codex review #379 round 1 caught a real correctness issue: the packageHTTPResponse calls in executeRequest + the prefetch path in dispatchItemUpdate passed the dispatcher's outer ctx instead of req.Context(). The lister reads CurrentUser + TokenAllowedWorkspaces from context, and the canonical "everything attached" context is the SYNTHESIZED request's context — buildHTTPRequest layers WithCurrentUser + WithAPITokenAuth on it, and d.Apply (when wired) attaches token state on top of req specifically. In production this happened to work because MCPBearerAuth attaches TokenAllowedWorkspaces on the inbound /mcp request's context, which the dispatcher inherits as its outer ctx. But: - Tests driving executeRequest with context.Background() + a UserResolver-supplied user got empty available_workspaces because the outer ctx had no user. - Any future dispatcher attaching token state via Apply (rather than relying on inbound-ctx propagation) would also see the bug — the Apply hook is documented as the place for "TASK-953 token-scope context" exactly. Fix: pass req.Context() / prefetchReq.Context() to packageHTTPResponse. Same dispatcher, same ServeHTTP — just feed the lister the canonical post-Apply context. Test: TestExecuteRequest_UsesRequestContext_NotOuterContext drives executeRequest with an empty outer context + a UserResolver, asserts the resulting unknown_workspace envelope has the user's full workspace list. With the buggy version the test fails (lister sees no user → empty hints).	2026-05-02 15:35:18 -04:00
xarmian	3319ad5ea1	feat(mcp): per-token rate limit on /mcp (TASK-959) (#378) ... * feat(mcp): per-token rate limit on /mcp (TASK-959) Add a per-token rate limit to /mcp's auth middleware. Closes the "runaway agent burns through user quota" gap that PLAN-943 left as a follow-up to TASK-950. ## Policy - 60 requests / minute / token, burst 20. - Per-token (not per-IP): office-NAT-shared users don't share a bucket, and a runaway agent on one token can't burn another token's quota for the same user. - Limiter key: SHA-256(bearer) — the raw token never lives in the limiter map even though buckets persist for the 5-minute retention window. - Discovery docs (`/.well-known/oauth-*`) are NOT rate-limited. They're polled by MCP clients before any token exists; rate- limiting them per-IP would penalize office NATs and per-bearer doesn't apply (no bearer to hash). - No-bearer requests are 401'd before the limiter sees them, so a bare-bones DoS via empty Authorization headers gets the cheap rejection path without sharing a (necessarily-empty) bucket key. ## 429 response Per RFC 6585: `Retry-After: <seconds>` header (computed from the limiter's refill rate), plus `X-RateLimit-Limit`, `X-RateLimit-Remaining: 0`. Body is the MCP-shaped JSON envelope `{"error": {"code": "rate_limited", "message": "..."}}` so MCP clients (Claude Desktop, Cursor) can surface the error consistently. ## Implementation - `RateLimiters.MCPPerToken` — new `*ipRateLimiter` instance, drained in `Stop()` so cleanup goroutines don't leak (BUG-851 pattern). - `Server.checkMCPRateLimit` — called from `MCPBearerAuth` BEFORE auth validation. Returns false + writes 429 when bucket is exhausted; auth still 401s if the token is also invalid (the rate limit and validity checks are independent). - `hashTokenForLimiter` — SHA-256 hex digest helper. Uniform with the limiter's other (IP-string) keys. - `writeMCPRateLimit` — emits the 429 envelope. ## Tests - TestMCPRateLimit_PerToken_BucketEnforced — single token → 429 within 30 attempts (60/min, burst 20). - TestMCPRateLimit_PerToken_TwoTokensIndependent — drain token1 to 429, verify token2 still passes a full burst. - TestMCPRateLimit_DiscoveryDocsExempt — 50 hits to /.well-known/oauth-protected-resource, zero 429s. - TestMCPRateLimit_NoBearer_NotCounted — no-bearer requests 401 before the limiter, no 429s. - TestMCPRateLimit_429EnvelopeShape — Retry-After, X-RateLimit-* headers, MCP error envelope shape. - TestHashTokenForLimiter — hash determinism, length, no collision by prefix, empty input safety. * fix(mcp): move per-token rate limit AFTER auth validation (Codex round 1) Codex review #378 round 1 caught a memory-DoS risk: the pre-auth limiter created a new bucket entry for every distinct bearer string. An attacker rotating random bearer values would grow the limiter map unbounded until the 5-minute cleanup tick — millions of phantom entries before the goroutine catches up. Fix: relocate the checkMCPRateLimit call to AFTER auth validation in both PAT and OAuth paths. The limiter map now only fills with hashes of *valid* tokens, bounding map size by the active-token count rather than by the bearer-string space. Trade-off: invalid-bearer spam still hits the auth path's DB lookup (CPU cost, but a single indexed read per request) without any rate limiting. The CPU exposure is small enough to accept for v1; a follow-up could add a pre-auth per-IP cap for invalid-token flooding if real abuse appears. Tests: - TestMCPRateLimit_InvalidBearerNotRateLimited — 50 invalid bearers in a row, none get 429 (always 401). - TestMCPRateLimit_LimiterMapBoundedByValidTokensOnly — direct regression: 100 distinct invalid bearers, limiter map size must NOT grow. - Existing happy-path tests updated to use real PATs (via the new mustCreatePATForTest helper) so the post-auth-validation guard doesn't short-circuit them. * fix(mcp): move OAuth rate limit AFTER all validation gates (Codex round 2) Codex review #378 round 2 caught a P3 gap in round 1's fix. The OAuth path's rate-limit check ran AFTER IntrospectToken but BEFORE: - access-token-vs-refresh-token check - RFC 8707 audience match - session.GetSubject() presence - GetUser lookup So an active-but-not-authorized OAuth bearer (refresh token used as a bearer, wrong-audience token, deleted user) would create a limiter entry. After 30 such requests the response would flip from the intended 401 invalid_token to 429 — leaking limiter state to attackers and slightly defeating the bounded-map property. Fix: move the OAuth-path checkMCPRateLimit call to the very end of handleMCPOAuthAuth, just before context attachment + next.ServeHTTP. Now the limiter map only contains tokens that would have reached the dispatcher otherwise. Test: TestMCPRateLimit_OAuthRefreshTokenNotCounted — mints a real refresh token via the full OAuth flow, hammers /mcp with it 50 times, asserts every response is 401 AND the limiter map size is unchanged. * fix(mcp): move PAT rate limit AFTER all validation gates (Codex round 3) Codex review #378 round 3 caught the symmetric issue in the PAT path that round 2 fixed for OAuth. checkMCPRateLimit ran AFTER ValidateToken but BEFORE: - apiToken.UserID == "" check (legacy workspace-scoped tokens) - GetUser lookup (deleted-user case) Active-but-not-authorized PAT bearers (legacy tokens with no user_id, tokens whose user was deleted) would have created limiter entries and eventually 429'd instead of returning the intended 401 invalid_token. Fix: move the PAT-path checkMCPRateLimit call to the very end of handleMCPPATAuth, just before context attachment + next.ServeHTTP. Now mirrors the OAuth path's positioning — both run the rate limit exactly once, at the END of their happy path, so the limiter map only contains tokens that would otherwise reach the dispatcher.	2026-05-02 15:15:36 -04:00
xarmian	d01bbf6bf1	feat(oauth): live workspace allow-list + role enforcement (TASK-953) (#377) ... Closes the third leg of PLAN-943's OAuth permission model: (token capability tier) × (live workspace role) × (consent allow-list) The first two were already in place — TASK-1027 wired the tier scope check (pad:read / pad:write / pad:admin via tokenScopeAllows) and RequireWorkspaceAccess does the live role lookup. This PR adds the third gate: the workspace-allow-list set at consent time (TASK-952) actually denies workspaces NOT in the user's selection. ## What's new - `oauth.Session.AllowedWorkspaces()` / `SetAllowedWorkspaces()` — typed accessors on session.Extra. Handle BOTH the in-memory []string shape (consent-decide path) AND the JSON-decoded []interface{} shape (post-storage round-trip path). - `WithTokenAllowedWorkspaces` / `TokenAllowedWorkspacesFromContext` — context helpers in internal/server with defensive copies so callers can't corrupt the per-request token state. - `MCPBearerAuth` (OAuth path) reads the token's allow-list from session.Extra and stashes it in context. - `RequireWorkspaceAccess` checks the allow-list against the resolved workspace's slug. Three behaviours match TokenAllowedWorkspacesFromContext's return shapes: - nil → no token-level gate (PAT auth, pre-TASK-952 OAuth tokens). Standard membership applies. - ["*"] → wildcard. Every membership the user has passes. - [slug-a, slug-b, ...] → only listed slugs. Anything else gets 403 permission_denied BEFORE the membership check. ## Live role + revocation Membership revocation takes effect immediately. RequireWorkspaceAccess calls GetWorkspaceMember on every request — if the user lost membership in workspace X, the token's allow-list including X no longer helps; the request is rejected at the standard membership gate. Tested explicitly via TestWorkspaceAllowList_LiveMembershipRevocation. ## Tier × role The natural intersection of tokenScopeAllows (tier-based HTTP-method gate) and per-handler role checks (e.g. requireEditPermission) handles the tier × role table from the PLAN-943 spec: - pad:write tier passes tokenScopeAllows for POST. - But Viewer role fails requireEditPermission's role check. - Net: 403 — tested explicitly via TestWorkspaceAllowList_TierTimesRole_WriteByViewer. ## Tests Unit (no I/O): - TestTokenAllowedWorkspaceMatches — policy table for the helper. - TestWithTokenAllowedWorkspaces_DefensiveCopy + 1 reader counterpart. - TestSession_AllowedWorkspaces_*: setter/getter, nil-clear, defensive copy, JSON round-trip ([]string + []interface{} branches), wildcard JSON round-trip, not-set, nil-session. Integration (full chain, real OAuth flow): - TestWorkspaceAllowList_AllowsListedSlug — listed workspace passes. - TestWorkspaceAllowList_DeniesUnlistedSlug — unlisted gets 403 even though user is owner. - TestWorkspaceAllowList_WildcardAllowsAnyMembership — wildcard passes for every membership. - TestWorkspaceAllowList_LiveMembershipRevocation — token works, then membership revoked, then same token denied. - TestWorkspaceAllowList_PATPathUnaffected — PAT regression: PATs don't carry an allow-list, must NOT hit the gate. - TestWorkspaceAllowList_TierTimesRole_WriteByViewer — pad:write tier × Viewer role on POST item → 403.	2026-05-02 14:29:17 -04:00
xarmian	7d0de978f7	feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) (#376) ... * feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) Replace the inline-HTML stub from sub-PR C (TASK-1025) with the real consent page described in PLAN-943: server-rendered HTML with workspace multi-select, "any workspace" wildcard, and a capability- tier radio (read / write / admin). ## What the page does - Lists every workspace the user is a member of, with their role shown next to each row (informational — TASK-953 does live role resolution at MCP-call time). - Wildcard checkbox grants "any workspace I currently or later have access to," with a clear warning when checked. Mutually exclusive with per-workspace boxes (vanilla JS for UX, server-side rejection as the security gate). - Capability tier radio is constrained to the intersection of {pad:read, pad:write, pad:admin} and the client's requested scopes — fosite's grant-time subset check (RFC 6749 §3.3) rejects scopes outside the request, so the UI must never offer them. Default selects the highest tier the client requested. - Allow button stays disabled until ≥1 workspace (or wildcard) is selected. Server-side validation enforces the same rule regardless of JS state. ## Selective consent This is the central security property. The decide handler now grants *exactly* the chosen tier scope, NOT every requested scope. If the client requests `pad:read pad:write` and the user picks "read", the issued token has `scope=pad:read` only. Bonus fix: removed redundant scope re-grant loop in handleOAuthToken that would have expanded granted scopes back to the full requested set on every /token exchange — a real security bug that the auto-approve stub from sub-PR C masked because granted == requested for that flow. fosite's flow_authorize_code_token.go:134-138 + flow_refresh.go:91-103 copy GrantedScope/Audience automatically; our loop was undoing selective consent. ## Workspace allow-list storage The user's workspace selections live in `session.Extra["allowed_workspaces"]` (round-trips via storage.go's existing JSON marshal). Either `["*"]` for wildcard or a list of slugs. fosite's WriteIntrospectionResponse serializes Extra into the introspection response as top-level fields, so TASK-953's enforcement layer reads them off `/oauth/introspect` (or in-process via fosite.IntrospectToken). This sidesteps fosite's strict "granted ⊆ requested ⊆ client.Scopes" check — clients don't request `pad:workspaces:foo`, but the consent UI lets the user pick from their workspaces regardless. TASK-953 implements the live role resolution + workspace gate. ## Defense in depth - Server validates `capability_tier ∈ {read, write, admin}` AND that the chosen tier is among the client's requested scopes — fosite would reject otherwise with a less-readable error. - Server validates every non-wildcard slug is in the user's current membership table. A tampered form sending other slugs gets 400. - Wildcard wins: if a tampered POST sends both `*` and specific slugs, the result is `["*"]` only — never partial allow-list. ## Tests - TestConsent_RendersUserWorkspaces — multi-workspace list with role labels. - TestConsent_NoWorkspaces_ShowsEmptyState — clean empty state. - TestConsent_TierRadios_OnlyRequestedScopes — UI hides tiers the client didn't request. - TestConsent_ApproveWithSpecificWorkspaces — happy path, asserts introspection returns `allowed_workspaces=[alpha, beta]`. - TestConsent_ApproveWithWildcard — wildcard yields `["*"]`. - TestConsent_ApproveWithoutWorkspaceSelection_Rejected — 400 on empty allow-list. - TestConsent_ApproveWithUntrustedSlug_Rejected — defense in depth. - TestConsent_ApproveWithUnrequestedTier_Rejected — server tier validation matches UI's tier-radio constraint. - TestConsent_TokenScopeMatchesTierChoice_Read — selective consent: user picks read-only despite client requesting both, token has exactly `pad:read`. Existing tests + helpers updated to include the new consent fields (`capability_tier`, `allowed_workspaces`). * fix(oauth): prevent URL parameter pollution attack on consent UI (round 1) Codex review #376 round 1 caught a P1 security bug in the consent UI. The hidden-input round-trip used the full r.URL.Query() with only `csrf_token` stripped, so a malicious OAuth client could craft /oauth/authorize?...&capability_tier=admin&allowed_workspaces=* and the consent form would render those as hidden inputs BEFORE the user-controlled radios + checkboxes. On submit, the hidden values precede the user's selection in the form encoding, so: - r.FormValue("capability_tier") returns "admin" (first value matches the attacker's, not the user's) - r.PostForm["allowed_workspaces"] sees "*" first, the wildcard scan matches, the result is ["*"] regardless of which boxes the user actually checked Net effect: a user clicking through the consent UI for "read-only, just my docapp workspace" would silently authorize "admin, all workspaces" — without any visible cue that the values were wrong. Fix: build hidden inputs from an explicit allowlist of OAuth-standard authorize-request parameters (response_type, client_id, redirect_uri, scope, state, audience, resource, code_challenge, code_challenge_method, nonce). Anything outside the allowlist is silently dropped. This is strictly stronger than blocklisting consent-control names, because it also defends against future OAuth extensions adding new attacker- controllable params we haven't enumerated. Test: TestConsent_URLPollution_DoesNotOverrideUserSelection simulates the attack — GET /authorize with attacker params, asserts the rendered HTML contains zero `<input type="hidden" name="<attacker_name>">`, then completes the flow with the user's actual selection and confirms the issued token's scope matches the user's choice (pad:read), not the attacker's URL injection (pad:admin).	2026-05-02 14:08:11 -04:00
xarmian	924d82dae4	feat(oauth): MCPBearerAuth OAuth integration + public-info (TASK-1027) — closes TASK-951 (#375) ... * feat(oauth): MCPBearerAuth OAuth integration + public-info endpoint (TASK-1027, sub-PR E of TASK-951) Closes the OAuth server build-out by connecting sub-PRs A-D to the MCP transport from TASK-950 and shipping the consent-screen support endpoint. ## MCPBearerAuth OAuth path middleware_mcp_auth.go now branches on token shape: - pad_<60-hex> → existing PAT validation (TASK-950 path) - anything else → fosite.IntrospectToken via the new internal/oauth.Server.IntrospectToken wrapper (server-side, no HTTP roundtrip — pad-cloud is both auth server and resource server, so the public /oauth/introspect endpoint is for external clients only). OAuth path validation gates: - Token must be active (fosite returns ErrInactiveToken / ErrNotFound on revoked / unknown / expired tokens). - tokenUse must be access_token; refresh tokens explicitly rejected (RFC 6749 §1.5 — refresh tokens aren't bearers for resource calls). - Granted audience MUST contain the canonical MCP URL (RFC 8707 anti-replay; resource-server-side check defends against compromised or shared auth servers). - Subject must resolve to a real user row. Successful path stashes user + scopes via WithCurrentUser / WithTokenScopes. Scopes are translated from fosite's space-separated form to JSON-array form via oauthScopesToJSON. ## tokenScopeAllows pad:* extension Extended to recognize the OAuth scope vocabulary alongside PAT scopes: - pad:read ↔ read (GET/HEAD/OPTIONS only) - pad:write ↔ write (all methods) - pad:admin ↔ * (all methods) So MCP tool authorization stays uniform regardless of which transport issued the bearer. ## /api/v1/oauth/clients/{id}/public-info New read-only endpoint for the consent screen (TASK-952) and the OAuth-intent banner (TASK-1001, already shipped). Returns four non-sensitive fields: client_id, client_name, logo_uri, redirect_uris. - Auth-required (any logged-in user). - Cloud-mode-gated (404s outside cloud). - 404 for unknown clients. - Whitelisted leak surface — explicit fields, no embedded models.OAuthClient, so a future field addition (e.g. a confidential- client secret) doesn't accidentally appear here. ## Tests - TestMCP_OAuthAccessToken_Authenticates — happy path: full flow yields a token that authenticates against /mcp. - TestMCP_OAuthAccessToken_AudienceMismatch_Rejected — RFC 8707 resource-server check; mints a token, swaps the OAuth server for one with a different canonical, confirms 401. - TestMCP_OAuthRefreshToken_RejectedAtMCP — refresh tokens MUST NOT authenticate. - TestMCP_RevokedOAuthToken_Rejected — revocation takes effect at the resource server. - TestMCP_PATPath_StillWorks — regression for sub-PR D's coexistence with the OAuth path. - TestMCP_OAuthScopeReadOnly_StashesPadReadScope — scope round-trip. - TestOAuthClientPublicInfo_HappyPath / UnknownClient_404 / Unauthenticated_401 / NotMountedOutsideCloudMode — full coverage of the new endpoint. - TestE2E_ClaudeDesktopFlow — simulates the full sequence (discovery → DCR → authorize → token → /mcp call) Claude Desktop walks on first connect. - TestTokenScopeAllows extended with pad:* coverage. ## TASK-951 status Closes TASK-951 when this lands (5/5 sub-PRs done): - A: schema + storage layer (#370 / 2a00775) - B: fosite-backed authorization-server constructor (#371 / f6eeee4) - C: DCR + authorize + token endpoints + populated discovery (#372 / 48776a3) - D: revoke + introspect endpoints (#373 / 4250fb1) - E: MCPBearerAuth + public-info (this PR) * fix(oauth): fail-closed on empty OAuth scopes per Codex review (round 1) Codex caught a high-severity bug in oauthScopesToJSON: the helper mapped empty granted scopes to `[]`, which tokenScopeAllows interprets as the legacy "unrestricted" PAT shape (allow all methods). Combined with OAuth's RFC 6749 §3.3 rule that the `scope` parameter is OPTIONAL, this meant a client could: 1. Run the auth-code flow without requesting scopes. 2. Get back a token with empty granted_scopes. 3. Drive write MCP tools because MCPBearerAuth stashed `[]` and tokenScopeAllows fell through to the legacy unrestricted path. Fix: map empty OAuth scopes to JSON `null` instead. tokenScopeAllows denies on the "scopes == nil" branch (existing TASK-667 behavior), so the entire write surface is denied for empty-scope OAuth tokens. In production this path is hard to hit — sub-PR C's DCR handler defaults registered clients to `pad:read pad:write` when omitted, and audienceMatchingStrategy enforces canonical-audience matching at grant time. Defense-in-depth at the resource server is the right policy regardless. Test: TestOAuthScopesToJSON_FailClosedOnEmpty asserts both halves of the contract — the helper produces "null" for empty input, and tokenScopeAllows denies every method when fed that value.	2026-05-02 13:33:53 -04:00
xarmian	4250fb1976	feat(oauth): revoke + introspect endpoints (TASK-1026) (#373) ... * feat(oauth): revoke + introspect endpoints (TASK-1026, sub-PR D of TASK-951) Add the RFC 7009 revocation and RFC 7662 introspection endpoints, completing the spec'd surface that sub-PR C left as placeholders. - POST /oauth/revoke — fosite NewRevocationRequest delegates to our storage adapter's RevokeRefreshToken / RevokeAccessToken which walk the request_id (grant family) and mark every chain member inactive in one statement. Public clients authenticate by sending only client_id (token_endpoint_auth_method=none). - POST /oauth/introspect — fosite NewIntrospectionRequest with Bearer auth (a separate active access token). Returns {active:true, sub, scope, aud, client_id, exp, iat} for active tokens; bare {active:false} for unknown/revoked/expired (RFC 7662 §2.2 no-leak rule). Sub-PR E's MCPBearerAuth integration uses fosite.IntrospectToken directly server-side, but the public endpoint satisfies the discovery contract for clients that follow the chain. - Discovery doc populates revocation_endpoint + introspection_endpoint and their auth_methods_supported lists ("none" for both — public-clients-only model). Tests cover: - /revoke marks an access token inactive (verified via introspect). - /revoke on a refresh token revokes the entire grant family (paired access also goes inactive). - Refresh-token rotation: old pair becomes inactive, new pair active. - Refresh-token replay detection: replaying a rotated refresh kills the family (OAuth 2.1 §6.1, RFC 6819 §5.2.2.3). - Introspect happy path returns sub/scope/aud/client_id/exp. - Introspect on unknown token returns just {active:false} with no field leakage. - Introspect rejects requests with no Bearer Authorization header. - /revoke + /introspect 404 outside cloud mode. - Discovery doc advertises both endpoints + auth-methods lists. * fix(oauth): drop introspection_endpoint_auth_methods_supported per Codex review (round 1) Codex review of #373 caught a contradiction in the discovery doc: introspection_endpoint_auth_methods_supported: ["none"] advertised "no client authentication" for the introspection endpoint, but fosite's NewIntrospectionRequest rejects a request without Authorization: Bearer ... (and the test in this PR locks that in). A discovery-driven client would treat "none" as "post token+client_id unauthenticated" and get 401 — worse than no advertisement at all. Fix: omit introspection_endpoint_auth_methods_supported entirely. RFC 8414 §2 marks the field OPTIONAL; omission tells clients to negotiate auth out-of-band, which for our public-clients-only model means "send a separate active access token in the Authorization header." We document that in getpad.dev/mcp/local. revocation_endpoint_auth_methods_supported = ["none"] is kept and honest — fosite's NewRevocationRequest really does accept a public client posting only client_id (no Bearer required). * fix(oauth): RFC 7009 §2.2 idempotent revoke per Codex review (round 2) Codex caught that fosite v0.49 returns ErrInvalidRequest for the unknown-token path of NewRevocationRequest, which WriteRevocationResponse turns into 400. RFC 7009 §2.2 explicitly requires: "The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token." The 400 break the entirely normal "client retried after a previous revoke succeeded" or "operator typo'd the token" cases. Fix: detect the bare ErrInvalidRequest from the !found branch via isRevocationUnknownToken (which inspects HintField — fosite sets the hint on every other ErrInvalidRequest path it returns from NewRevocationRequest) and write 200 directly. Genuine malformed requests (wrong method, unparseable body, empty form) still return 400 because their ErrInvalidRequest carries a hint. Tests: - TestOAuth_Revoke_UnknownToken_Returns200 — locks in 200 for the unknown-token path. - TestOAuth_Revoke_MalformedRequest_Returns400 — counterpart that ensures the 200 override doesn't accidentally swallow real malformed-request errors. * fix(oauth): require token param + remove dead-code revoke override (round 3) Codex round 3 noticed that POST /oauth/revoke with client_id but no token returned 200 OK — silently swallowing a missing-required- parameter error. RFC 7009 §2.1 marks `token` REQUIRED. Investigating the fix surfaced that round 2's isRevocationUnknownToken override was actually dead code: fosite v0.49's handler/oauth2/revocation.go's RevokeToken collapses ErrNotFound + ErrInactiveToken to nil via storeErrorsToRevocationError, so NewRevocationRequest returns nil and WriteRevocationResponse writes 200 natively for unknown tokens. The override never fired in any real path. Cleanup: - Replace the unused isRevocationUnknownToken + override with a pre-check that returns 400 invalid_request when `token` is missing. RFC 7009 §2.1 enforced; fosite's native idempotency handles unknown tokens. - Update TestOAuth_Revoke_UnknownToken_Returns200's comment to reflect that it pins fosite's native behavior (not our override). - Add TestOAuth_Revoke_MissingToken_Returns400 to lock in the pre-check. - Keep TestOAuth_Revoke_MalformedRequest_Returns400 — verifies fosite's own ErrInvalidRequest paths still surface as 400.	2026-05-02 12:47:54 -04:00
xarmian	48776a3967	feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372) ... * feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951) Third of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. Mounts the three flow-driving HTTP endpoints over the fosite-backed server constructed in sub-PR B, replaces the TASK-950 501 stub with the real RFC 8414 discovery doc, and ships an inline-HTML consent stub as a TASK-952 placeholder so the auth-code flow runs end-to-end. What lands: - internal/server/handlers_oauth.go (744 LoC) - POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite. Public clients only (token_endpoint_auth_method=none rejected for any other value), authorization_code + refresh_token grants only, code response type only. Validates redirect_uris (absolute, no fragment, https or loopback-http or custom- scheme like claude://, blocks file:/javascript:/data:/vbscript:). - GET /oauth/authorize: starts auth-code flow. fosite validates request shape (PKCE-S256 required, audience matched, redirect exact-match). If user has session → renders inline consent stub. If not → 302 to /login?redirect=<self> (TASK-998's plumbing in pad-cloud honors the redirect=). - POST /oauth/authorize/decide: processes consent decision. Form-bound CSRF token (the existing __Host-pad_csrf cookie, read from a hidden form field instead of header). Approve → fosite NewAuthorizeResponse → 303 to client.redirect_uri with code. Deny → fosite WriteAuthorizeError(access_denied). - POST /oauth/token: code + refresh exchange. fosite verifies PKCE verifier (S256-required) + RFC 8707 audience. Returns {access_token, token_type, expires_in, refresh_token, scope}. RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS issues on authorize-code grant. - Inline consent stub: minimal HTML form with Approve/Deny, auto-grants every requested scope (TASK-952's UI replaces with workspace allow-list selection per TASK-953). - internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub → handleOAuthAuthorizationServer. Returns RFC 8414 metadata with all six endpoint URLs (revoke + introspect URLs sub-PR D fills with handlers; the URLs are stable now), advertised scopes, S256-only code_challenge_methods, resource_indicators_supported=true, authorization_response_iss_parameter_supported=true. - internal/server/server.go: Server.oauthServer field + SetOAuthServer + registerOAuthRoutes called from setupRouter inside an r.Group with requireCloudMode + SessionAuth (so /authorize can detect the logged-in user via __Host-pad_session; SessionAuth falls through gracefully when no cookie). - cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL being set (the OAuth surface needs a canonical audience to bind tokens to). CSRF posture: middleware_csrf.go runs only on /api/* paths so /oauth/* is naturally exempt. The consent decision endpoint adds its own form-token check (validateConsentCSRFToken) using the same __Host-pad_csrf cookie the SPA uses, just with the token in a hidden form field rather than a header. Same security model, different transport. Tests (12, all passing): - TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins RFC 8414 metadata fields including S256-only PKCE + resource_indicators_supported. - DCR (5): happy path; missing redirect_uris; bad redirect-URI shapes (relative, non-loopback http, fragment, javascript:); non-public client auth method rejected; unknown grant type rejected; not mounted outside cloud mode. - /authorize (3): redirects to /login when no session; renders consent stub when logged in; rejects audience mismatch via fosite's audienceMatchingStrategy. - /authorize/decide (2): rejects missing csrf_token; deny produces access_denied redirect. - Full PKCE flow: end-to-end /authorize/decide (approve) → /token with code_verifier → 200 with access+refresh tokens. - /token: rejects missing PKCE verifier. Replaces the 501 stub assertion in TestMCP_AuthServerStub with TestMCP_AuthServerMetadata_Mounted (just confirms 200; full shape lives in the OAuth-handler test). Out of scope: - /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026) - MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027) - Real consent UI with workspace allow-list (TASK-952) * fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1) Two findings from PR #372 round 1: 1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT) send `resource=` not `audience=`. fosite v0.49 reads only `audience` from the form, so audienceMatchingStrategy was hit with an empty needle and rejected every real-world authorize / token request. Tests masked the gap by sending both keys. Fix: translateResourceToAudience() copies r.Form["resource"] into r.Form["audience"] before each handler invokes fosite. Idempotent — if both keys are present, audience wins (test harness sends both for belt-and-suspenders). Applied at /authorize, /authorize/decide, and /token entry points. Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY resource= (no audience=) and asserts the request reaches the consent stub. Without the translation it 303s with invalid_request. 2. P2: /.well-known/oauth-authorization-server advertised /oauth/revoke + /oauth/introspect endpoints that don't exist yet (sub-PR D wires them). Real clients dialing those URLs would get 404. RFC 8414 §2 lists revocation_endpoint + introspection_endpoint as OPTIONAL, so omitting until the handlers ship is spec-compliant + honest. Fix: drop revocation_endpoint, introspection_endpoint, and their *_endpoint_auth_methods_supported counterparts from authServerMetadata. Sub-PR D's PR description includes "populate these here" as a follow-up. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints asserts the four fields are absent. * fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2) Two findings from PR #372 round 2: 1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop / Cursor self-register without prior auth) but had no rate limit. An attacker could flood the oauth_clients table indefinitely. Fix: extend RateLimit middleware to gate /oauth/register at the same 5/hour/IP rate the existing /api/v1/auth/register uses (RateLimiters.Register, burst 5). Added the OAuth route group to the s.RateLimit middleware chain so the new path actually runs through the limiter. Other /oauth/* endpoints aren't rate-limited here: /authorize rides session cookies (cheap to abuse but ineffective without a logged-in user), /token is PKCE-bound to a stored code (single-use), /authorize/decide is form-bound. Explicit per- endpoint /oauth/* limits arrive with TASK-959. Test TestOAuth_Register_RateLimited fires 5 requests successfully, asserts the 6th returns 429. 2. P2: Discovery doc advertised authorization_response_iss_parameter_supported=true, but the /authorize success path delegates to fosite v0.49 which doesn't add iss=<issuer> to the redirect. RFC 9207-aware clients seeing the flag would treat the missing parameter as a protocol violation. Fix: drop the field from authServerMetadata. RFC 8414 §2 marks it OPTIONAL — omission is spec-compliant. We'll add the parameter (+ post-processing of fosite's response) in a future PR if a real client requires it; today's MCP clients (Claude Desktop, Cursor, ChatGPT) don't. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints extended to cover the field. * fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3) Codex round 3 caught: /.well-known/oauth-authorization-server lives in the MCP route group (registerMCPRoutes), while the /oauth/{ register,authorize,token} handlers live in the OAuth route group (registerOAuthRoutes, gated on s.oauthServer != nil). A cloud deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted but NOT OAuth — the discovery doc would 200 with /oauth/* URLs that 404. Worse for clients than no document at all. Fix: handleOAuthAuthorizationServer now also nil-checks s.oauthServer; on nil it returns 503 with config_error, matching the existing fail-loud branch for when the issuer URL isn't configured. Ops detect the misconfiguration immediately rather than fielding "OAuth registration is failing with 404" tickets. Test: - TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled builds a Server with SetCloudMode + SetMCPTransport (so the MCP route group mounts) but NOT SetOAuthServer; asserts the endpoint returns 503 with config_error. - TestMCP_AuthServerMetadata_Mounted renamed → TestMCP_AuthServerMetadata_MountedAndGated to reflect the new behavior under mcpEnabledTestServer (which doesn't wire OAuth). The full 200 happy path lives in TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses oauthEnabledTestServer). * fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known * fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485 CI govulncheck rejected the build: fosite v0.49.0 transitively pulls github.com/go-jose/go-jose/v3@v3.0.3 which has GO-2025-3485 (DoS in JWS parsing). Affected call site: internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned. Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory). go mod tidy auto-bumped dependent indirect deps too. Verified locally: govulncheck ./... → "No vulnerabilities found" go test ./... → all green go build ./... → clean	2026-05-02 11:56:57 -04:00
xarmian	f6eeee4f81	feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951) (#371) ... * feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951) Second of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. Wires fosite v0.49.0 over the storage layer from sub-PR A. No HTTP routes yet — sub-PR C mounts /authorize, /token, /register; sub-PR D mounts /revoke and /introspect. What lands: - internal/oauth/session.go — pad's *Session embedding fosite.DefaultSession with typed UserID() accessor + Clone override returning *Session (so handler-side type-assertions don't lose the concrete type during refresh-token rotation). - internal/oauth/storage.go — Storage adapter satisfying: fosite.ClientManager handler/oauth2.AuthorizeCodeStorage handler/oauth2.AccessTokenStorage handler/oauth2.RefreshTokenStorage handler/oauth2.TokenRevocationStorage handler/pkce.PKCERequestStorage Compile-time guards in server.go assert each interface remains satisfied. Translation: fosite.Requester ⇄ models.OAuthRequest via JSON-encoded session_data + URL-encoded form. Sentinel errors from sub-PR A map to fosite.ErrNotFound / ErrInvalidatedAuthorizeCode / ErrInactiveToken. - internal/oauth/audience.go — RFC 8707 custom AudienceMatchingStrategy. fosite has no native RFC 8707; we close over a canonical audience and reject any request that doesn't carry exactly that resource. Belt-and-suspenders haystack check defends against fixtures / migrations that register a client without setting Audience. Plus ValidateAudienceParam (HTTP-handler entry helper) and audienceForNewClient (DCR seed for sub-PR C). - internal/oauth/server.go — NewServer(Config) → *Server returning fosite.OAuth2Provider configured for: - PKCE-S256 required (EnforcePKCE + EnablePKCEPlainChallengeMethod=false) - Opaque HMAC tokens (compose.NewOAuth2HMACStrategy) - Refresh rotation with grant-family revocation (sub-PR A round-2 fix) - Audience binding via the custom strategy Sensible default lifespans (1h access, 30d refresh, 15m authcode); overridable via Config. Excluded by design: client-credentials, implicit, ROPC (deprecated in OAuth 2.1), OpenID factories (we're not OIDC), PAR (not needed for v1). - go.mod — github.com/ory/fosite pinned at v0.49.0 (direct dep). Tests (20): - NewServer required-field validation (3) + default-lifespan path - audienceMatchingStrategy: empty needle, mismatch, client-without-canonical, canonical-only happy path, multi-audience rejection, no-canonical=ServerError - ValidateAudienceParam (5 sub-cases) + audienceForNewClient - Session: clone returns concrete *Session (not *DefaultSession); nil-safe accessors - Storage adapter: auth-code round-trip + invalidated-code error, GetClient not-found mapping, access-token-inactive mapping, rotation-revokes-entire-grant (end-to-end), PKCE round-trip, requester-to-OAuthRequest session encoding + missing-client guard Out of scope (subsequent sub-PRs): - HTTP route handlers + DCR endpoint (sub-PR C / TASK-1025) - /revoke + /introspect endpoints (sub-PR D / TASK-1026) - MCPBearerAuth OAuth introspection branch (sub-PR E / TASK-1027) * fix(oauth): inject canonical audience into hydrated clients per Codex review (round 1) Codex round 1 caught a P1 in the storage adapter: modelClientToFosite returned fosite.DefaultClient.Audience=nil for every persisted client, but audienceMatchingStrategy's haystack-side check requires client.GetAudience() to contain the canonical audience. Net result: every authorize / token / refresh flow would fail with invalid_request once the strategy ran, regardless of how the client was registered. Fix: thread the canonical audience through Storage. NewStorage now takes a canonicalAudience string; modelClientToFosite (now a method on Storage) injects [canonicalAudience] into the hydrated client's Audience field. The audience isn't persisted as a column — single-resource AS for v1 (PLAN-943) means every client implicitly allows the same audience, so storing what we'd always set to the same value is pure write amplification. Threading: cfg.AllowedAudience → NewServer → NewStorage(store, audience) └─ Storage.canonicalAudience └─ modelClientToFosite injects Misconfigured Storage (empty canonicalAudience — caught earlier by NewServer's required-field check, but tests pin the fail-loud branch in case Storage is ever constructed directly): produces clients with Audience=nil so audienceMatchingStrategy rejects every request with ServerError, surfacing the misconfiguration fast rather than silently issuing wide-open tokens. Tests: - TestStorage_GetClient_InjectsCanonicalAudience — pins the injection contract; without the fix this fails. - TestStorage_NewStorage_EmptyCanonicalLeavesAudienceNil — pins the fail-loud branch for misconfigured Storage. - 6 existing tests updated to pass canonical audience to NewStorage (mechanical sed update; behaviour unchanged). * fix(oauth): hydrate request payload on inactive token Get*Session per Codex review (round 2) Codex round 2 caught a HIGH-severity gap: GetRefreshTokenSession returned (nil, fosite.ErrInactiveToken) for revoked rows, but fosite's handleRefreshTokenReuse (flow_refresh.go:178-204) derefs req.GetID() to drive the family revocation that's the OAuth 2.1 BCP §4.14 replay-detection rule. Returning nil nil-derefs that flow and defeats replay detection — the very thing rotation exists to enable. Fix: hydrate the stored row even on the inactive path and return (req, fosite.ErrInactiveToken). Mirrors the pattern already used by GetAuthorizeCodeSession's invalidated-code branch. If hydration itself fails (client deleted between issuance and use), return the underlying error rather than masking it — replay detection loses but the failure is observable. Symmetric fix applied to GetAccessTokenSession even though no fosite caller currently derefs on inactive there. Defense in depth + uniform contract makes the adapter resilient to future fosite changes (e.g. an introspector that wants req.GetID() for audit-log enrichment). Tests: - TestStorage_GetRefreshTokenSession_InactiveReturnsPayload — pins the refresh-side contract; without the fix this fails on the nil-check. - TestStorage_GetAccessTokenSession_InactiveReturnsPayload — same pattern for access tokens. * fix(oauth): set RefreshTokenScopes=[] so authorize-code grants issue refresh per Codex review (round 3) Codex round 3 caught a P1: fosite defaults Config.RefreshTokenScopes to ["offline", "offline_access"]. fosite only mints refresh tokens when one of the listed scopes is granted. PLAN-943's scope vocabulary is pad:read / pad:write / pad:admin — no "offline" scope — so the default silently disabled refresh issuance for every Pad grant, defeating the entire refresh-rotation + family-revocation machinery this PR adds. Fix: explicitly set RefreshTokenScopes: []string{} in NewServer's fosite.Config. fosite reads the empty slice as "issue refresh on every authorize-code grant whose client allows the refresh_token grant type, no scope predicate" — matches fosite's own tests (flow_authorize_code_token_test.go:129). Pin: TestNewServer_RefreshTokenScopesIsEmpty documents the decision + smoke-checks that the constructor still returns a usable provider. The actual "refresh issued on authorize-code grant" assertion lands in sub-PR C's /token endpoint test — that's where fosite's flow_authorize_code_token.go reads the field. * fix(oauth): bump go.opentelemetry.io/otel{,/sdk} to v1.40.0 to resolve GO-2026-4394 CI govulncheck job rejected the build: fosite v0.49.0 transitively pulls in go.opentelemetry.io/otel/sdk@v1.21.0 which has known vulnerability GO-2026-4394 (Arbitrary Code Execution via PATH Hijacking in go.opentelemetry.io/otel/sdk). Affected init-time call sites: internal/oauth/audience.go:8 → fosite.init → otel resource.init internal/server/middleware_ratelimit.go:80 → sync.Once.Do → resource.Default internal/cli/client.go:415,794,798 → otelhttp.* → trace.* Fix: bump otel core + sdk + metric + trace to v1.40.0 (the fixed version per GO-2026-4394's advisory). go mod tidy also pulled in go.opentelemetry.io/auto/sdk@v1.2.1 as a new transitive. Verified locally: govulncheck ./... → "No vulnerabilities found" go test ./... → all green go build ./... → clean	2026-05-02 10:59:07 -04:00
xarmian	2a00775481	feat(oauth): schema + storage layer (TASK-1023, sub-PR A of TASK-951) (#370) ... * feat(oauth): schema + storage layer for OAuth 2.1 server (TASK-1023, sub-PR A of TASK-951) First of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. This one is foundation only — no HTTP exposure, no fosite import, no public surface change. Schema (5 tables, parallel SQLite + Postgres migrations): - oauth_clients — RFC 7591 Dynamic Client Registration; public clients only for v1 - oauth_authorization_codes — short-lived codes for the auth-code grant - oauth_access_tokens — opaque HMAC; subject denormalized for fast user-bound queries - oauth_refresh_tokens — same shape; access_token_signature link + request_id chain - oauth_pkce_requests — PKCE session keyed by auth-code signature Storage layer (internal/store/oauth.go): - 12 public methods covering fosite's ClientManager + CoreStorage + PKCERequestStorage + TokenRevocationStorage interface shapes, using pad-internal types so the package stays fosite-free. - Three sentinel errors (ErrOAuthNotFound, ErrOAuthInvalidatedCode, ErrOAuthInactiveToken) that sub-PR B's adapter maps to the matching fosite errors. - request_id IS the chain identifier (fosite preserves it across rotations — handler/oauth2/flow_refresh.go:86), so family revocation is a single indexed UPDATE rather than a separate chain_id column. 14 tests covering: client CRUD + idempotent delete + empty-slice normalization, auth-code create/get/invalidate (including the "return payload alongside ErrInvalidatedCode" contract fosite relies on for family revocation), access-token CRUD + delete, refresh CRUD + RotateRefreshToken (single-row flip), refresh-token family revocation (entire chain via request_id, leaves other chains untouched), access-token family revocation, PKCE CRUD, and required-field validation. Both backends share test bodies via testStore(t); set PAD_TEST_POSTGRES_URL=... to run the same suite against Postgres. Out of scope for this PR (subsequent sub-PRs): - fosite import + adapter (sub-PR B / TASK-1024) - DCR + authorize + token endpoints (sub-PR C / TASK-1025) - revoke + introspect endpoints (sub-PR D / TASK-1026) - MCPBearerAuth OAuth integration (sub-PR E / TASK-1027) * fix(oauth): always insert active=true; drop broken zero-value Active override per Codex review (round 1) Codex round 1 caught a P1 in insertOAuthRequestRow: active := defaultActive if req.Active != defaultActive { active = req.Active // <- zero-value collides } When defaultActive=true and req.Active=false (the zero value), this branch fires and the row is stored with active=FALSE — silently producing immediately-revoked tokens. Any sub-PR B adapter that built an OAuthRequest without explicitly setting Active=true would ship broken. Fix: hardcode active=TRUE on insert. Drop the defaultActive parameter (it's always true for the three flagged tables; PKCE has no active column). Pre-seeding inactive isn't a supported flow — fosite never does it, and tests that need a revoked row do Create + Invalidate / Rotate / RevokeFamily as a two-step. Regression test TestOAuth_Insert_AlwaysActive constructs an OAuthRequest with zero-value Active and asserts the row is readable as active for all three table types (codes, access, refresh). Without the fix the test fails on the first GetAccessToken call with ErrOAuthInactiveToken. * fix(oauth): RotateRefreshToken revokes both refresh + access families per Codex review (round 2) Codex round 2 caught: my RotateRefreshToken only marked the named refresh row inactive, but fosite's reference MemoryStore.RotateRefreshToken (storage/memory.go:497-504) revokes BOTH the refresh family AND the access family for the grant's request_id. Without this, every access token issued before a refresh remained active until TTL — defeating the rotation's invalidation contract. Fix: RotateRefreshToken now delegates to RevokeRefreshTokenFamily + RevokeAccessTokenFamily (both already existed). The signatureToRotate parameter becomes vestigial — fosite passes it but the family revoke catches every chain member regardless of which row triggered the rotation. The new pair fosite immediately issues via CreateAccessTokenSession + CreateRefreshTokenSession inherits the same request_id (flow_refresh.go:86) and lands active=TRUE per the round-1 hardcode, so the net post-rotation state is "all old rows in this grant inactive, the new pair active." Test rewrite: TestOAuth_RotateRefreshToken_FlipsActiveOnSingleRow asserted the OPPOSITE behavior (only one row touched) — that was the original bug. Replaced with TestOAuth_RotateRefreshToken_RevokesEntireGrant which seeds a refresh + access pair in the same chain, plus a distinct unrelated grant, then asserts after rotation: - old refresh + old access both inactive - unrelated grant untouched (request_id-scoped) * fix(oauth): DeleteOAuthClient cascades dependent rows in a tx per Codex review (round 3) Round 3 finding: DeleteOAuthClient errored with FK constraint violation for any client that had ever issued a grant. The migrations declare client_id FKs without ON DELETE CASCADE — by design, so a stray DELETE FROM oauth_clients elsewhere fails loudly rather than silently nuking grants — but that meant the "officially supported" delete path was unusable. Fix: DeleteOAuthClient now runs five sequential DELETEs inside a single transaction: 1. oauth_pkce_requests 2. oauth_refresh_tokens 3. oauth_access_tokens 4. oauth_authorization_codes 5. oauth_clients Order matters (children before parent) because the FKs aren't cascading. The tx makes it atomic — if any step fails, nothing's deleted, so we never leave a half-deleted client. Idempotent because every WHERE matches nothing on a non-existent client. Test TestOAuth_DeleteOAuthClient_CascadesDependentRows seeds a row in each of the four dependent tables, deletes the client, and asserts ErrOAuthNotFound on every dependent row + the client itself. Without the fix this fails on the first DELETE FROM oauth_clients with an FK constraint violation. * fix(oauth): SELECT FOR UPDATE row lock in DeleteOAuthClient on Postgres per Codex review (round 4) Codex round 4 caught a Postgres race in DeleteOAuthClient: the five-DELETE cascade is atomic, but between the child-row deletes and the parent delete, a concurrent fosite handler can insert a fresh grant/token referencing the same client_id. The parent DELETE then fails with an FK violation and the whole tx rolls back — the cascade is correct, but unreliable under concurrent OAuth issuance. Fix: take SELECT id FROM oauth_clients WHERE id = ? FOR UPDATE as the very first statement in the tx (Postgres only). The exclusive row-level lock blocks any concurrent statement that tries to read the client row — which fosite does on FK resolution during grant/token inserts — until our tx commits. Skipped on SQLite because: (a) BEGIN IMMEDIATE serializes the entire write workload globally (DSN configures _txlock=immediate per store.go), so the race doesn't exist. (b) FOR UPDATE syntax isn't reliably accepted across SQLite drivers. ErrNoRows on the lock query is treated as "client doesn't exist yet" — the subsequent DELETEs match nothing and the call remains idempotent. Tests still pass on the SQLite path; the Postgres path's race fix will be exercised by CI's PAD_TEST_POSTGRES_URL runs and any future concurrency test we add.	2026-05-02 10:25:53 -04:00
xarmian	521853e0a1	feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369) ... * feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied.	2026-05-02 09:25:25 -04:00
xarmian	48bbe7453b	fix(dashboard): suggested_next surfaces in-progress + filters blocked items (BUG-990) (#366) ... Pre-fix algorithm only considered status == "open" child items in active plans. Two consequences agents flagged in BUG-987 / BUG-990: 1. In-progress items never appeared. The most likely "what should I work on next" answer is "the work the user is already on" — pre-fix the engine returned [] when nothing was open AND a task was actively in-progress. 2. Blocked items appeared. Suggesting work that has unresolved blockers wastes the user's time when they go to start it. Algorithm changes (internal/server/handlers_dashboard.go): - Include both `open` and active-status (in-progress / fixing / exploring / etc., via existing isActiveStatus helper) child items. - Filter out items with at least one active "blocks" link from a non-done blocker. Mirrors the attention-section logic, factored into a new itemBlockedByActive helper. - Sort: in-progress first (always wins over open, regardless of priority), then by priority rank within each bucket. Pinned in test expectations. - Reason text distinguishes "In-progress task..." vs "Open task..." so agents see why an item was suggested. Tests: - TestDashboardSuggestedNext expectations updated for new ordering (in-progress wins over open). Test now covers the in-progress surfacing path the bug specifically wanted. - New TestDashboardSuggestedNext_FiltersBlockedItems exercises the blocker-filter — explicitly creates a blocks link and confirms the blocked task is suppressed even at critical priority. Out of scope: high-priority orphan suggestions (items not in any active plan). The bug item flagged this as a stretch goal; this PR sticks to the active-plan-children scope of the existing algorithm. Adding orphans would need a sort/relevance model since "everything high priority" can be a long list. Parent: BUG-990.	2026-05-01 21:48:04 -04:00
xarmian	9657051e43	fix(mcp): dedup implementation_notes / decision_log from fields blob (BUG-992) (#365) ... Item responses carry implementation_notes and decision_log in TWO places: 1. Top-level arrays on the item (item.ImplementationNotes, item.DecisionLog) — populated by hydrateItemComputedMetadata. 2. Inside the stringified `fields` blob — written there by AppendImplementationNote / AppendDecisionLogEntry at write time. This duplicate forces agents to dedup or pick a source; the bug report's recommendation was to keep the top-level arrays as the canonical shape and drop the embed. Path-consistent with BUG-991 path A (also MCP-only normalization): extend the boundary normalizer in packageJSONResult so that AFTER fields is parsed (BUG-991), implementation_notes and decision_log keys are dropped from the parsed fields object. The top-level arrays continue to surface unchanged. Server / web / CLI keep their existing behavior — fields still carries the embed at rest, hydration still extracts to top-level. The boundary fix is the cheap clean-up; a write-side migration (stop persisting into the fields blob, plus a one-shot data migration to clean existing items) is the architecturally proper fix and remains tracked in BUG-992's notes. Tests: - internal/mcp/bug992_test.go: stripDuplicatedFieldsKeys helper (strips both keys, no-op when absent, defensive on non-object inputs) + end-to-end packageJSONResult cases for single-item and array-style responses. Parent: BUG-992.	2026-05-01 21:40:01 -04:00
xarmian	708897dd0c	fix(mcp): parse fields/tags at MCP boundary so agents see native shapes (BUG-991 path A) (#364) ... Item responses carry `fields` and `tags` as JSON-stringified strings because the underlying SQLite columns store them that way. For agents going through MCP this means a double-encode every read — they have to JSON.parse the field's string value before doing anything useful. Path A (this PR): normalize at the MCP boundary. Recursively walk parsed JSON in packageJSONResult, find string-typed `fields` and `tags` properties, parse the embedded JSON, substitute the native shape. Server / web / CLI keep their existing stringified contract; agents see clean JSON. The walk handles every common item shape: - Single-item responses (top-level item) - Item arrays (item list, dashboard.active_items, comment lists) - Nested items (dashboard.recent_activity[].item, parent_*) Conservative parse: only strings starting with `{` or `[` and successfully parseable as JSON get substituted. Hand-written values that happen to share a key name (e.g. a `fields` description text) pass through untouched. Malformed JSON also passes through as the original string rather than dropping the value. Text fallback (content[0].text on the MCP result) preserves the original CLI body verbatim. Older clients that read the text content keep seeing the same shape — the structured wire is the agent upgrade path; text is the back-compat path. Same pattern as BUG-985's {items: [...]} array wrap. Path B (full migration of models.Item.Fields from string to map[string]any across server/store/web/CLI) is tracked in BUG-991's notes — bigger surgery, deferred. Tests: - internal/mcp/bug991_test.go: single-item, item-array, nested-item, primitives-pass-through, malformed-stays-string, plus end-to-end packageJSONResult cases proving structured + text branches both work. - Existing dispatch_http_advanced_test.go and dispatch_http_project_test.go updated: tests that previously did `json.Unmarshal([]byte(fieldsStr))` now use a small itemFieldsAsMap helper that reads the parsed map directly. Cleaner, and a clear error message if normalization regresses. Parent: BUG-991.	2026-05-01 21:35:29 -04:00
xarmian	55d3a078a8	fix(mcp): standup CLI ref + classifier polish for BUG-987 round 2 (#362) ... Round-2 hotfix on top of PR #361 (which shipped to v0.1.0-rc.4). Claude Desktop's re-review of rc.4 surfaced two fixes that didn't fully land: - Bug 8 (round 1 went to wrong layer). My HTTPHandlerDispatcher fix populated ref on standup blockers, but Claude Desktop's path is ExecDispatcher → CLI subprocess → standupCmd, which has its own JSON composition struct. That struct's Attention + SuggestedNext anonymous types didn't even define ItemRef as a parseable field. Now both define `item_ref` and the JSON-emit loops set Ref from it. Verified live: blockers now carry refs (TASK-X), not empty strings. - Bug 11 part 2. Round 1 stripped the cobra Usage block but two artifacts still leaked: 1. The "pad <verb> failed: <stderr>" prefix on server_error fallback messages. The verb name is the OLD CLI verb (e.g. `pad item block`) which doesn't match the v0.2 catalog actions agents see, and the cmdPath is already implicit from the invoked tool. Drop the prefix; emit the cleaned stderr directly. 2. Self-link / "cannot ..." validation rejections classified as server_error instead of validation_failed. Extended the validation regex with `cannot ` so server-side rejections like "cannot link an item to itself" / "cannot modify archived item" route to ErrValidationFailed. Verified live with a self-link attempt — now returns code=validation_failed, hint="cannot link an item to itself", no prefix. - New stripErrorPrefix helper trims leading `Error:` / `error:` / `ERROR:` from every classified hint+message so the envelope text isn't redundant with the envelope's `code` signal. Bug 13 / Bug 14: my round-1 fixes verified working locally on rc.4 (tested with a fresh Task → convention=None; dashboard by_role shows "Unassigned"/"unassigned" for the bucket). The reviewer's stale results almost certainly reflect a pad server process that wasn't restarted with the rc.4 binary swap. Tests: - TestClassifyExecError_CannotPhrasingClassifiesAsValidation — three "cannot ..." stderr cases must classify validation_failed. - TestClassifyExecError_NoLegacyVerbPrefixInMessage — pins the prefix-strip behaviour on the server_error fallback path. - TestStripErrorPrefix — trim-rule round-trip across casing variations and empty input. Parent: BUG-987.	2026-05-01 21:04:26 -04:00