mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-21 18:13:26 +00:00
v0.2.0
542 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
6c44291e78 |
fix(layout): let Cmd+F fall through to browser-native find on item views (BUG-986) (#423)
The layout's global keydown handler unconditionally intercepted Cmd+F and routed it through a `collectionSearchRequested` boolean that only the collection list page polled. On item / document views (and any non-collection page) nothing watched the flag, but `e.preventDefault()` had already blocked the browser's native find — leaving users with no way to search inside a document. Invert the model from "always intercept, broadcast a flag" to "only intercept when a page registers a handler": - ui.svelte.ts: replace `collectionSearchRequested` with a `collectionSearchHandler` registry exposing `registerCollectionSearch` / `unregisterCollectionSearch` / `triggerCollectionSearch` / `hasCollectionSearchHandler`. - +layout.svelte: only `e.preventDefault()` and dispatch when `uiStore.hasCollectionSearchHandler` is true; otherwise let Cmd+F pass through to the browser. - [collection]/+page.svelte: register the existing filters-open + focus-search behaviour in an `$effect` and unregister it via the effect's cleanup so it lives only while the page is mounted. Result: collection list view keeps its existing Cmd+F filter-search shortcut; item views and any other page get the browser's native find back. `make check` clean (Go tests, lint, web build, svelte-check 0 errors).v0.2.0 |
||
|
xarmian
|
50d04944c5 |
feat(sweep): gate role board + child reorder + run grep verification (TASK-1108) (#422)
* feat(sweep): gate role board + child reorder + run grep verification (TASK-1108)
Final sweep across PLAN-1100 (client-side permission audit). Confirms no
remaining open-coded permission checks and gates the few remaining surfaces
not covered by tasks 1102-1107.
Code-only acceptance grep results:
- `members.find(...)` outside workspaceStore: ZERO matches
- `m.role === 'owner' / 'editor'` open-codes outside permissions.ts: ZERO matches
- `isOwner = $derived(workspaceMembers...)` open-codes: ZERO matches
Surfaces gated in this PR:
- Role board (`/{workspace}/roles`):
- "+ New" item button gated on canEditAnyItem (owner|editor)
- "+ Add Role" column owner-only
- Lane edit button (✎) owner-only, lane drag handle owner-only
- Lane-header drag (column reorder) owner-only — handlers and draggable
attribute conditional on isOwner
- Lane-items dndzone receives dragDisabled: !canEditAnyItem (zone-level
library limitation, same constraint as TASK-1106)
- ChildItems component (used on item detail to render children):
- New canEdit prop (default true). Slug page passes canEdit (= canEditItem
of parent). dndzone receives dragDisabled: !canEdit so non-editors
can't drag-reorder children.
BUG-984 closure remains gated on HT-1157 (manual three-role smoke test),
which will sign off on real walks as owner / editor / viewer / guest.
Parent: PLAN-1100.
* fix(sweep): empty-state role create gate + grant-aware canEditAnyItem per Codex review (round 1)
Two findings from round 1:
1. Empty-state "Create your first role" button at roles/+page.svelte:530
wasn't gated. Now wrapped in {#if isOwner}.
2. canEditAnyItem was role-only (owner|editor). Server's reorder handler
is grant-aware per-item, so a viewer/guest with even one
CollectionGrant.edit or ItemGrant.edit can legitimately mutate via the
role board. Helper now ORs in any active edit grant — matches what
the server enforces.
Parent: PLAN-1100. Refs TASK-1108 PR #422.
* fix(sweep): align role-board create/lane-reorder gates with server per Codex review (round 2)
Two findings from round 2:
1. The role-board "+ New" item flow was gated on canEditAnyItem (any role
or any edit grant), but server's handleCreateItem requires collection-
level edit (collection grant or role+visibility), not item-only grant.
Item-grant-only users would see "+ New" but get a 403 on submit.
Filter eligibleCollections by workspaceStore.canEditCollection(coll.id)
and gate the button on eligibleCollections.length > 0.
2. Lane reorder was owner-only, but server's handleRoleBoardLaneReorder
uses requireMinRole "editor". Editors lost an allowed operation.
Introduced canReorderLanes (owner | editor) for lane drag handles +
ondrag handlers; role create/edit/delete remain owner-only.
Parent: PLAN-1100. Refs TASK-1108 PR #422.
|
||
|
xarmian
|
7f06a9845a |
feat(comments): gate composer / replies / reactions / delete on canEditItem (TASK-1107) (#421)
Comment timeline previously rendered composer, reply, reaction picker,
and per-reaction toggle for all roles. Server enforces edit per-item on
comment writes; UI now matches.
Note on file paths: TASK-1107's original spec referenced
`web/src/lib/components/comments/CommentThread.svelte`, but that file is
unused anywhere in `web/src/`. Real comment UI lives in:
- `ItemTimeline.svelte` (composer + thread layout)
- `TimelineCommentCard.svelte` (per-comment delete, replies, reactions)
Changes:
- ItemTimeline: imports workspaceStore, accepts itemId + collectionId
props (optional with safe fallback), derives canEdit reactively. The
composer is hidden entirely when !canEdit.
- TimelineCommentCard: accepts canEdit prop. Delete / reply / reaction
picker / reply-comment delete / reply reaction picker all gated.
- Existing reactions still render with counts so read-only viewers see
who reacted; the chip's onclick is gated and disabled={!canEdit} so
toggling is blocked for them.
- Slug page passes item.id + item.collection_id so ItemTimeline can
resolve canEditItem itself (no prop-drilling of canEdit).
Parent: PLAN-1100.
|
||
|
xarmian
|
fe9b76ab93 |
feat(views): gate drag/archive in ListView/BoardView on canEditCollection (TASK-1106) (#420)
The collection page's ListView and BoardView allowed all roles to drag
items, drag-status-change, reorder groups/columns, and archive groups.
Server enforces edit per-item and per-collection on these mutations
(handlers_items.go, handlers_role_board.go) — UI now matches.
Changes:
- ListView + BoardView accept a `canEdit?: boolean` prop (default true to
preserve behavior in existing callers).
- ListView: dndzone for groups + intra-group items receives
`dragDisabled: !canEdit`. Group drag handle and archive-group button
hidden when !canEdit.
- BoardView: column-cards dndzone receives `dragDisabled: isMobile || !canEdit`.
Column-header drag (column reorder) gated via `draggable={canEdit}` and
conditional drag handlers. Column-drag-handle indicator and
archive-column button hidden when !canEdit.
- Collection page passes `canEdit={canEditThisCollection}` to both views.
Scope note: per-item drag gating (e.g. a guest with ItemGrant.edit on one
item dragging just that one card) is not implemented — svelte-dnd-action
only supports zone-level dragDisabled. Achieving per-item would require
switching to dragHandleZone+dragHandle and shipping an explicit handle UI
for everyone, which is a larger UX change. Server already enforces per-item
edit on the resulting mutations, so no security gap. Documented as a
follow-up if needed.
TableView: excluded from drag/archive scope — no drag handlers to gate.
Status-cell editing is already gated via FieldEditor's readonly prop from
TASK-1105.
Parent: PLAN-1100.
|
||
|
xarmian
|
a8b158829b |
feat(item-detail): gate write affordances on canEditItem (TASK-1105) (#419)
* feat(item-detail): gate write affordances on canEditItem (TASK-1105)
Item detail page hides title-edit, content editing, FieldEditor inputs,
delete button, and assignment dropdowns when the user lacks edit on this
specific item. Mirrors the server's per-item permission cascade so the UI
cannot show affordances the server would 403.
Per-item gate via workspaceStore.canEditItem(item) — owner → item grant →
collection grant → role + visibility → deny. Handles guests with single
ItemGrant.edit (full edit on that one item, read-only on siblings) and
the precedence regression where ItemGrant.view + CollectionGrant.edit on
the same item resolves to read-only (item grant wins per server cascade).
Changes:
- FieldEditor: new `readonly?: boolean` prop. When true, renders a unified
display block per field type (select / checkbox / date / number / url /
text) — same visual language as the editor's idle state, no inputs, no
dropdowns, no mutation handlers. Documented in the component header.
- RawMarkdownEditor: new `readonly?: boolean` prop, applied to the
underlying textarea.
- [slug]/+page.svelte: derived canEdit predicate. Title swaps from
click-to-edit button to plain h1 when read-only. Editor passes
editable=canEdit; EditorBubbleMenu / EditorLinkPopover only mount when
editable. RawMarkdownEditor passes readonly. Delete button hidden.
FieldEditor receives readonly={!canEdit}. Assignment + role dropdowns
swap to read-only display spans.
- New CSS: .title-readonly (no hover, default cursor),
.assignment-readonly (matches assignment-select height for layout
stability when the user gains/loses edit permission).
Parent: PLAN-1100.
* fix(item-detail): gate Editor toolbars + ?new=1 title bypass per Codex review (round 2)
Two read-only escape hatches found by Codex re-review:
1. Editor.svelte mobile toolbar (line 818) and table toolbar (line 846)
rendered without checking the `editable` prop. tiptap's editor instance
correctly refuses commands when editable=false, so the buttons would
no-op, but they still rendered and were visually misleading. Both
toolbars now gated on `editable`.
2. The slug page's auto-start-title-edit path for ?new=1 didn't check
canEdit. A read-only user appending ?new=1 would land on the title
textarea (which the visible-branch gate now hides). Added canEdit to
the auto-start condition AND to startEditTitle() itself as a defensive
second line.
Round 1 disagreements stand: Move-to / item-links / ChildItems are
explicitly TASK-1108 sweep scope and intentionally not addressed here.
Parent: PLAN-1100. Refs TASK-1105 PR #419.
* fix(item-detail): exclude BlockDragHandle in read-only + gate Move-to / links per Codex review (round 3)
Three findings from round 3:
1. Editor's BlockDragHandle ProseMirror plugin (registered in Editor's
extensions list) is not gated by tiptap's `editable` flag — its drag
handle is injected into the view DOM regardless. A read-only user
could drag blocks to dispatch transactions through onUpdate. Fix:
conditionally include the plugin in the extensions array based on
`editable`.
2 + 3. Move-to button and item-links add/delete affordances. These were
originally TASK-1108 sweep scope, but Codex re-flagged them in
round 3 despite the round-2 deferral. Absorbed into TASK-1105
rather than burn more review rounds — the gating is mechanical
(a few {#if canEdit} wrappers). TASK-1108 sweep will still grep
for any remaining open-coded patterns elsewhere.
Parent: PLAN-1100. Refs TASK-1105 PR #419.
* fix(item-detail): re-key Editor on canEdit change so BlockDragHandle reattaches per Codex review (round 4)
Round 3 excluded BlockDragHandle from the editor's extensions array when
editable=false. Round 4 caught the construction-time-only nature of that
gate: on cold/direct navigation /me resolves after the editor mounts, so
canEdit starts false → editor created without BlockDragHandle → /me
resolves → canEdit flips true but the existing $effect only calls
editor.setEditable(true) and does not re-register extensions.
Fix: add canEdit to the {#key} value so the editor is reconstructed when
permission flips. Cost is a brief loss of cursor/scroll position on the
flip — acceptable since the only path that flips canEdit mid-session is
a grant change while the page is open, which is rare.
Same approach is appropriate for any future extension whose registration
is gated on `editable`.
Parent: PLAN-1100. Refs TASK-1105 PR #419.
* fix(item-detail): handle ?new=1 auto-edit reactively for slow /me per Codex review (round 5)
* fix(item-detail): always reassign pendingNewItemEdit per Codex review (round 6)
|
||
|
xarmian
|
d311245654 |
feat(collection-page): gate item-create affordances on canEditCollection (TASK-1104) (#418)
The collection list page renders multiple "create item" affordances — header "+ New" button, quick-create input, empty-state CTA, and view-level buttons via EmptyState — to all roles regardless of whether they can actually create items in this collection. Server rejects the writes; this hides the affordances entirely. Per-collection gate via workspaceStore.canEditCollection(collection.id) — not the binary canEdit. A viewer with a CollectionGrant.edit on Tasks sees "+ New" on /tasks but not on /ideas. A guest with only an ItemGrant sees no create affordance anywhere (server cascade: item grant doesn't promote to collection-wide write). Changes: - Header "+ New" button: hidden when !canEditThisCollection. - Quick-create input: only renders when both quickCreateOpen AND canEditThisCollection (defensive, since openQuickCreate is no longer callable through any visible affordance). - Empty-state-box CTA: hidden when !canEditThisCollection. Message also switches from "Create your first ..." to "This collection is empty." - View-level oncreate prop: undefined when !canEditThisCollection, so EmptyState (the shared empty-state component) hides its own create button automatically. Parent: PLAN-1100. |
||
|
xarmian
|
0035a9dad9 |
feat(settings): gate collection management UI to owners (TASK-1103) (#417)
Settings → Collections currently shows "+ Create Collection" and clickable edit cards to all roles. The server already enforces owner-only on create, update, and delete (handlers_collections.go:48, :113, :164). UI now matches. Changes: - Collection cards remain clickable for owners (open EditCollectionModal); for non-owners they render as non-interactive divs with the same content visible. The "Edit" hint is hidden for non-owners. - "+ Create Collection" button hidden entirely for non-owners. - CreateCollectionModal / EditCollectionModal mount only for owners — a non-owner can't reach them via the UI. Note: TASK-1103 spec floated "create gated to editor+", but the server is owner-only. Aligned UI to server (server is the security boundary). Parent: PLAN-1100. |
||
|
xarmian
|
3524a5ed92 |
feat(settings): gate Danger Zone tab + General write affordances on owner role (TASK-1102) (#416)
The presenting symptom of BUG-984: editors and viewers currently see the Danger Zone tab + workspace name/context/export controls. All of those are owner-only on the server. Gates them in the UI so the affordances aren't rendered to begin with. Changes: - Tabs are now derived: Danger Zone is filtered out for non-owners. Direct URL access to #danger as a non-owner snaps back to General. - Hash-driven tab restoration deferred to a validTabIds-aware $effect so owners deep-linking to #danger don't land on General because /me was still in flight at mount time. - General tab → Name input rendered readonly for non-owners; Save button hidden. - General tab → Context JSON textarea rendered readonly for non-owners; Save / Reset / Clear buttons hidden. - General tab → Export bundle gated to editor+ (canExport). Theme toggle remains available to all roles (personal preference, not workspace state). Owner experience unchanged. Non-owners now see a read-only General tab with workspace context visible (so they know what they're working in) but no controls that would 403. Parent: PLAN-1100. |
||
|
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.
|
||
|
xarmian
|
504e22d7bc |
fix(console): add mobile hamburger menu + scrollable admin tabs (BUG-1118) (#414)
The /console navbar's horizontal pill row crammed/clipped on narrow viewports, and the admin sub-tab strip wrapped awkwardly. Add a hamburger menu that toggles a dropdown panel below the navbar on mobile, and make the admin tab strip horizontally scrollable on the same breakpoint. Console layout (web/src/routes/console/+layout.svelte): - Hamburger button (32x32) appears in .nav-left on mobile (<=640px), switches to an X when open. Same SVG/sizing as TopBar.svelte's .mobile-hamburger so the chrome stays consistent. - .nav-links becomes a full-width dropdown panel below the navbar when open. Visual style mirrors TopBar.svelte's .user-dropdown (--bg-secondary, border, --radius-lg, box-shadow, dropdown-in keyframe). - Closes on link click, Escape, outside-click, and route change. - Route-change auto-close kept as its own single-purpose $effect per CONVE-606. - a11y: aria-expanded, aria-controls, aria-label on toggle; role=menu on panel, role=menuitem on links. - Desktop layout (>640px) is unchanged. Admin layout (web/src/routes/console/admin/+layout.svelte): - On <=640px the .admin-tabs strip becomes overflow-x: auto with -webkit-overflow-scrolling: touch, scrollbar-width: none, and flex-wrap: nowrap so all tabs are reachable without clipping. - Tabs stay flex-shrink: 0 + nowrap to keep labels readable. - Active-tab underline + colors preserved. |
||
|
xarmian
|
63d113624c |
fix(cli): retry password prompt on weak/mismatched passwords (BUG-1155) (#413)
* fix(cli): retry password prompt on weak/mismatched passwords during admin bootstrap (BUG-1155)
`pad auth setup` and `pad init` collected admin credentials with a single-
shot prompt: any rejection — local password mismatch, or server-side weak-
password / length error from validatePasswordStrength — bubbled up and
exited the command. The user had to re-run the whole flow (and in `pad
init`, redo configure + server-start) over a typo.
Replaces promptForAccountDetails() with promptAndBootstrap(client) which
collects email + name once, then loops the password / confirm pair (up to
5 attempts) on:
- local password mismatch
- *cli.APIError from /auth/bootstrap (covers all three messages from
internal/server/password_strength.go: too short, too long, too weak)
Network failures and other non-API errors still bail immediately.
Both call sites — cmd/pad/main.go (auth setup) and cmd/pad/init.go (init
step 3) — now use the new helper.
* fix(cli): only retry password-strength rejections, not all API errors per Codex review (round 1)
Round 1 retried on every *cli.APIError from /auth/bootstrap, but only
password-strength rejections are fixable by re-prompting the password
pair. The server also emits validation_error for invalid email / missing
name, conflict ("Pad instance has already been initialized"), and
forbidden (non-loopback bootstrap) — re-prompting just the password for
those traps the user in a 5-attempt loop that can never succeed.
Narrows the retry gate to validation_error whose message begins with
"Password" — the three messages emitted by validatePasswordStrength
(internal/server/password_strength.go: too-short, too-long, too-weak).
All other APIError codes and message shapes now fall through to the
fail-fast branch, so the user sees the real reason and can re-run with
the right correction.
|
||
|
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. |
||
|
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.
|
||
|
xarmian
|
278d051eb0 |
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. |
||
|
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. |
||
|
xarmian
|
553a39f09b |
fix(cli): pad auth setup hint should point at pad init, not a nonexistent IDEA-1 (TASK-1143) (#407)
PR #403 (TASK-1134) added printIdeaOneTriggerHint() to the pad auth setup success path so freshly-bootstrapped admins would learn about the seeded onboarding entry point. But pad auth setup only creates the first admin account — no workspace. IDEA-1 is only seeded when a startup-template workspace is created (via pad init / pad workspace init). A user following the original hint immediately would hit "workspace not found" / "item not found". Caught by Codex during review of PR #406 (the docs PR for TASK-1138). TASK-1143 was spawned then to keep PR #406 docs-only; this is the fix. Reframe (Option 2 from the task spec): keep the hint, but point at the next concrete action — `pad init` — rather than at IDEA-1. The IDEA-1 trigger phrase still surfaces in `printOnboardingHints`, which runs after `pad init` / `pad workspace init`. By then the workspace exists and the trigger phrase resolves correctly. Renamed `printIdeaOneTriggerHint` → `printPostSetupNextStepsHint` since the hint no longer names IDEA-1 directly. Wording matches CLAUDE.md / README — workspace creation precedes the trigger phrase everywhere. Parent: PLAN-1131 (follow-up). Origin: Codex review of PR #406 round 1. |
||
|
xarmian
|
d1fb61097e |
docs(onboarding): document IDEA-1 trigger phrase across README, CLAUDE.md, and /pad skill (TASK-1138) (#406)
* docs(onboarding): document the IDEA-1 trigger phrase across README, CLAUDE.md, and the /pad skill (TASK-1138) Make the seeded onboarding entry point (PLAN-1131) discoverable in every doc surface a fresh user might land on. README.md Quick Start gains a follow-up paragraph after `pad init`. Names the trigger phrase verbatim so a copy-paste lands deterministically. Tone matches in-product hint copy from PR #403; no "tutorial" / "lesson" language. CLAUDE.md Authentication section gets a paragraph after `pad auth setup` pointing developers + agents at the same trigger phrase. Also enumerates the four seeded refs (IDEA-1 / PLAN-2 / TASK-3 / DOC-4) for context, with pointers to the source-of-truth code (internal/collections/templates_onboarding.go) and design history (PLAN-1131). skills/pad/SKILL.md Adds a bullet under the Onboarding routing section: an explicit "use pad to get IDEA-1" trigger and the schema-aware terminal-status guidance per collection (Ideas → implemented, Plans → completed, Tasks → done, Docs → archived). Frames the seed items as ordinary items the agent reads and acts on — no "onboarding mode" — so the no-marker / no-skill-detection design from PLAN-1131 stays clean. pad-web (../pad-web) is intentionally not touched — separate repo per CONVE-159. Spawned TASK-1142 to pick up the pad-web getting-started flow as a follow-up. Parent: PLAN-1131. Origin: IDEA-1128. * fix(docs): scope the IDEA-1 hint to post-workspace-creation, not bootstrap setup, per Codex review (round 1) Codex caught that the original wording suggested users could go straight to `use pad to get IDEA-1` after `pad auth setup`. But `pad auth setup` only creates the first admin account — no workspace. IDEA-1 is only seeded when a `startup`-template workspace is created (`pad init` or `pad workspace init`). Tightened to call out the precondition explicitly: a startup-template workspace must exist before the trigger phrase resolves. Spawned TASK-1143 to fix the matching CLI hint behavior — PR #403's `printIdeaOneTriggerHint` after `pad auth setup` has the same imprecision and should either drop the IDEA-1 mention or point users at `pad init` first. Out of scope for this docs PR. |
||
|
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.
|
||
|
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.
|
||
|
xarmian
|
0a5eb777b9 |
feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134) (#403)
* feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134)
Make the seeded onboarding entry point discoverable without prior
knowledge. CONVE-191 calls for full-stack thinking on user-facing
features — this lands on every surface a fresh user might check.
CLI surfaces:
• `pad auth setup` success message gains a closing hint pointing at
`use pad to get IDEA-1` in a new agent session. New helper
printIdeaOneTriggerHint() so future templates can reuse the shape.
• `printOnboardingHints` (used after `pad init` / workspace creation)
now leads with the trigger phrase before the existing /pad prompt
suggestions. IDEA-1 is named because it's the seeded primary entry
in software-category templates; people-category templates will
seed REQ-1 / APP-1 etc. and need a template-aware version of this
hint — tracked under PLAN-1140.
Web UI surfaces:
• New OnboardingIdeaBanner component renders on the workspace
dashboard whenever IDEA-1 is in status=new. Shows the trigger
phrase verbatim with a copy button and a "Read it first" deep link
into the seeded item itself. Disappears the moment the user (or
agent) flips IDEA-1 out of `new`.
• Dashboard fetches IDEA-1 alongside its existing dashboard +
collections calls (cheap, indexed by ref) and re-checks on every
poll (default 30s) plus every sync signal so the banner is
self-correcting.
• Existing OnboardingChecklist gate (`totalItems === 0`) is left
alone. It still serves empty / non-templated workspaces; the new
banner is the templated-workspace surface.
No tests added — both surfaces are pure copy/render. Existing
dashboard + auth-setup tests still pass.
Parent: PLAN-1131. Origin: IDEA-1128.
* fix(onboarding): pin IDEA-1 lookup to exact prefix+number match per Codex review (round 1)
Server-side ResolveItem (via GetItemByRef) falls back from PREFIX-NUMBER
to a number-only lookup when the prefix doesn't match any collection in
the workspace. That fallback exists so an item moved between collections
is still resolvable by its old ref — but it has a bad interaction with
my new dashboard lookup:
In a non-software-category workspace (hiring, interviewing, …), there
is no Ideas collection. `api.items.get(ws, 'IDEA-1')` would silently
return whatever item has item_number=1 — typically REQ-1 (Requisition)
or APP-1 (Application). If that item happened to have status=new
(which the seeded Requisition / Application entries do), the dashboard
would render the IDEA-1 onboarding banner pointing at a /ideas/... URL
that 404s.
Fix: verify item.collection_prefix === 'IDEA' && item.item_number === 1
before trusting the result. Mismatch (or missing) → ideaOneStatus = null,
banner stays hidden. Software workspaces with a real IDEA-1 still match;
hiring / interviewing / interview-loop-style workspaces stop seeing the
banner entirely.
Caught by Codex on PR #403.
* fix(onboarding): guard IDEA-1 lookup against stale-workspace writes per Codex review (round 2)
Previous round addressed the wrong-collection match. This round fixes a
related race: rapid workspace navigation could let a slow loadIdeaOne()
from workspace A resolve after the user is already on workspace B and
write A's status into B's state, briefly rendering the IDEA-1 banner on
a workspace that doesn't have it.
Two-part fix:
1. The dashboard $effect that triggers load() now resets
ideaOneStatus = null synchronously when wsSlug changes, so any
leftover `new` status from the previous workspace can't briefly
render the banner during the window between navigation and the new
fetch resolving.
2. loadIdeaOne() now compares its captured slug against the current
wsSlug at every assignment point (success and error paths). If
they've diverged, the response is dropped — only the active
workspace's request can write ideaOneStatus.
Standard "was this still the active request" pattern. No behavior
change for the common case (single-workspace dashboard); the guard
only fires when navigation interleaves with an in-flight fetch.
Caught by Codex on PR #403.
|
||
|
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. |
||
|
xarmian
|
95025793b9 |
feat(mobile): consolidate topbar + search palette UX (IDEA-1121) (#401)
Mobile chrome was previously split: a full <TopBar mobile /> (logo +
switcher + avatar) when the sidebar was open, and a slim inline
.mobile-header (hamburger + switcher) when it was closed. Every
mobile-chrome feature had to be added in two places, and the original
ask — a search button — surfaced the architectural debt.
Consolidated to a single always-rendered mobile chrome:
- TopBar.svelte mobile branch: PadLogo replaced with a hamburger that
toggles the sidebar; new search-icon button calls openSearch() AND
onNavigate() so the sidebar closes before navigating to a result
(caught by Codex review, mirrors the desktop sidebar pattern).
- +layout.svelte: dropped the &&sidebarOpen gate so TopBar always
renders on mobile; deleted the inline .mobile-header and its CSS;
added padding-top: var(--topbar-height) on .app-layout via @media
(max-width: 768px) so content doesn't slide under the fixed bar.
- [collection]/[slug]/+page.svelte: removed the now-stale 45px sticky
offset that was pushing the breadcrumb below the deleted slim
header.
Search palette mobile UX (CommandPalette.svelte, all in one
@media (max-width: 768px) block — desktop is byte-identical):
- Full-screen takeover (100dvh, no max-width / shadow / radius) so
input anchors at top instead of fighting a vertically-centered
layout against the on-screen keyboard.
- 16px input font to suppress iOS Safari focus-zoom.
- X close button (.mobile-close) replacing the useless 'esc' kbd hint.
- Body-scroll lock effect (overflow: hidden only — touch-action: none
would have killed child scroll).
- .results pinned as the sole scroll target with flex: 1; min-height: 0
so the search input stays at the top regardless of result-list size.
Editor toolbar leak fix (Editor.svelte): the .mobile-toolbar (z-index
100) rendered whenever the on-screen keyboard appeared for ANY input
— including the global search palette on a page with a tiptap editor
mounted. Gated the render condition on editorFocused (already tracked
via editor.on('focus')/on('blur')) so the toolbar only appears when
the editor itself is focused.
Refs: IDEA-1121, TASK-1122, TASK-1124
|
||
|
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. |
||
|
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). |
||
|
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).
|
||
|
xarmian
|
d6c0073409 |
chore(web): point ConnectMCPModal docs link at /mcp/remote (TASK-1117 follow-up) (#397)
The connect-MCP modal had DOCS_HREF set to a temporary fallback at getpad.dev/docs/mcp because the canonical /mcp/remote landing didn't exist yet (TODO comment noted that). pad-web PR #80 (TASK-1117) just shipped /mcp/remote as the proper sibling to /mcp/local. Update the link target to match. The previous URL /docs/mcp now 404s on getpad.dev (page <24h old when moved; redirect explicitly waived per the project owner). This commit ensures every Pad instance points at the live URL going forward. Parent: PLAN-1111. Companion to pad-web #80. |
||
|
xarmian
|
78ec39daa2 |
feat(web): add ConnectMCPModal + wire into ConnectBanner (TASK-1115) (#396)
* feat(web): add ConnectMCPModal + wire it into ConnectBanner (TASK-1115)
Ships the Remote MCP onboarding modal that the MCP-mode banner has been
waiting for. With this PR, on any deployment that exposes a public MCP
URL (Pad Cloud + any self-host with PAD_MCP_PUBLIC_URL set), users with
an empty workspace see:
- A "Connect an AI agent — zero install →" banner (TASK-1114)
- Click → ConnectMCPModal with:
* The canonical MCP URL in a copy-block (sourced from
authStore.mcpPublicUrl, never hardcoded — works for self-hosted
deploys too)
* Four client cards (Claude Desktop, Cursor, Windsurf, ChatGPT) each
linking to the existing getpad.dev/docs/mcp/<client> page
* Footer links: Connected agents (in-app), Documentation
(getpad.dev/docs/mcp — TASK-1117 will swap to /mcp/remote when
that page lands), and "Prefer the CLI? →" which closes this modal
and opens the existing CLI install modal
ConnectBanner now mounts BOTH modals with independent open states; the
visibility predicate ORs them so the banner hides during interaction.
The "Prefer the CLI?" cross-link calls a parent callback so the banner
owns both states — ConnectMCPModal never directly mounts the CLI modal.
The transitional `mode === 'cli'` visibility gate from TASK-1114 is
removed (the gate's reason for existing — no MCP modal — is gone).
Validated:
- Svelte autofixer: 0 issues / 0 suggestions on the new component
- make check: 0 errors, 703 files (was 702 — confirms new file is
picked up by svelte-check)
Parent: PLAN-1111. Depends on TASK-1114 (banner refactor — shipped).
* fix(web): refetch on CLI-modal close regardless of banner mode (Codex round 1)
Codex caught a real bug: Effect C was gated on `mode === 'cli'`, but
the new MCP modal can flip the user to the CLI flow via "Prefer the
CLI? →". In that path, mode stays 'mcp' but the user runs `pad init`
and closes the CLI modal — and Effect C wouldn't refetch, leaving the
banner stale until a route change.
Fix: track `prevCliOpen` specifically and refetch on its true → false
transition, regardless of banner mode. The MCP-modal close transition
is still no-op (correct — user is off in a separate agent client).
|
||
|
xarmian
|
393d8f1d7d |
feat(web): ConnectBanner two-mode refactor (CLI / MCP) (TASK-1114) (#395)
* feat(web): ConnectBanner two-mode refactor (CLI / MCP) (TASK-1114)
Adds mode-aware rendering to the connect banner. When the server exposes
a Remote MCP URL via /auth/session.mcp_public_url (Pad Cloud + any
self-host with PAD_MCP_PUBLIC_URL set), the banner renders in MCP mode:
- Plug icon (vs the historical terminal-arrow)
- Copy: "Connect an AI agent to this workspace — zero install →"
- CTA: "Connect" (vs "Get the CLI")
Self-hosted instances without an MCP public URL keep the existing
CLI-mode copy + flow (regression-safe — no behavior change there).
Effect C (refetch on modal close) now runs CLI-mode only. In MCP mode
the user leaves the page entirely — off to Claude Desktop / Cursor /
Windsurf to paste the URL — so refetching the dashboard right after
modal close doesn't help. Effect B (workspace-change refetch) and the
SSE feed catch the first MCP-sourced item on the next visit.
localStorage dismiss key migration: writes now go to
`pad-connect-banner-dismissed-{ws}` (was `pad-cli-banner-dismissed-{ws}`).
Reads OR the new and legacy keys for one release as a soft migration so
existing dismissals carry over without re-pestering. Legacy key is left
in localStorage as harmless dead state — we don't own the cleanup path.
Transitional state: MCP-mode banner currently routes to the existing
ConnectWorkspaceModal as a fallback. TASK-1115 ships ConnectMCPModal
and will swap the binding. Until then, MCP-mode users who click see the
CLI install flow — worse UX than the destination, but coherent (no
broken click). Clearly TODO'd in the markup.
Validated with the Svelte autofixer (0 issues; advisory suggestions
about $effect usage are justified — localStorage reads + async fetches
+ previous-value tracking can't be expressed as $derived).
Parent: PLAN-1111. Depends on TASK-1112 + TASK-1113 (both shipped).
* fix(web): suppress MCP-mode banner until ConnectMCPModal ships per Codex review (round 1)
Codex P1: the MCP-mode copy promises "zero install" but the click still
opens ConnectWorkspaceModal (CLI flow), which is misleading for users
who land in that state.
Fix: gate `visible` on `mode === 'cli'` for now. The mode-detection,
branched copy/icon/CTA, and dismiss-key migration all stay — they're
ready to light up when TASK-1115 mounts the new modal. The transitional
gate is removed in TASK-1115 along with the modal swap.
Net effect this PR: cloud / MCP-exposed deploys see no banner at all
(strictly safer than misleading); self-hosted deploys are unchanged
(same CLI banner + flow).
Codex finding addressed: PR #395 round 1.
|
||
|
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.
|
||
|
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. |
||
|
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.v0.1.0 |
||
|
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.
|
||
|
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.
|
||
|
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.
|
||
|
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: ..."
|
||
|
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
|
||
|
xarmian
|
8cdf582066 |
fix(auth): full-page navigation for server-owned post-auth redirect targets (BUG-1083) (#386)
Unauthenticated users hitting /oauth/authorize were 302'd through /login, but the post-login goto(redirectTarget) used SvelteKit's client-side router to navigate back to /oauth/authorize — a Go-server route with no SPA match. SvelteKit fell into the [username]/[workspace] catchall, parsed it as username="oauth" + workspace="authorize", and rendered "No dashboard data available." Add isServerOwnedPath() + navigateToRedirectTarget() helpers in web/src/lib/auth/redirect.ts. The helper picks window.location.replace for paths the Go server owns (/oauth/, /api/, /.well-known/, /mcp, /metrics) and goto() for genuine SPA routes. window.location.replace matches the prior replaceState: true semantics so back-button doesn't return to /login. Swap all 5 post-auth call sites in login/+page.svelte (onMount, password submit, 2FA verify) and register/+page.svelte (onMount, register submit) to use the helper. goto import is no longer needed in either file. |
||
|
xarmian
|
7bb9076ac5 |
fix(dockerfile): pass version metadata via build args instead of broken in-container substitution (TASK-1080) (#385)
The previous build line had:
-X main.commit=$(git rev-parse --short HEAD 2>/dev/null) \
-X main.buildTime=$(date -u +%Y-%m-%dT%H:%M:%SZ)
The `git rev-parse` ran INSIDE the build container, but .dockerignore
intentionally excludes `.git/` so the working directory has no git
metadata; the `2>/dev/null` swallowed the resulting error and the
substitution silently produced an empty string. Net result: every
Docker build of pad shipped a binary with `commit=""`, which
fullVersion() collapses to just `version` ("dev") with no commit
metadata at all. Caught during Claude Desktop dogfooding when
`pad_meta` returned `pad_version: "dev"` and bug reports had no way
to identify which build they were hitting.
Two ways to fix: add .git/ to the build context (rejected — operator
explicitly does not want .git in the image build), or pre-compute on
the host and pass via --build-arg (this PR). The wrapper that does
the host-side computation lands in pad-cloud separately.
The `date` substitution worked because alpine has `date` in the
builder image, but it had a worse problem: a fresh `date` value on
every build invalidates layer caching for this RUN. Moving to ARG
lets the caller decide cache semantics — production wrappers will
pass a real timestamp; dev rebuilds can omit BUILD_TIME entirely
to keep the cache warm.
Defaults are deliberately ugly-but-honest so a `docker build .`
without args produces "dev (unknown)" rather than hiding the
misconfiguration. Production builds with all three args produce
e.g. "0.1.0-rc.5 (
|
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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=.
|
||
|
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).
|
||
|
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.
|
||
|
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.
|
||
|
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).
|
||
|
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 /
|
||
|
xarmian
|
cf04e16b5e |
chore(e2e): blog screenshot capture spec + shared seed helpers (TASK-1031) (#374)
Adds infrastructure for capturing Pad UI screenshots that ship inside
blog posts on getpad.dev.
* web/e2e/lib/demo-seed.ts (new) — extracts the realistic-content seed
(1 active plan + 7 tasks + 2 ideas) from screenshots.spec.ts into a
shared module, plus two new helpers:
- seedConventions(fixture, request, [...])
- activateLibraryConventions(fixture, request, titles)
Both consumers now share the same source of truth.
* web/e2e/blog-screenshots.spec.ts (new) — gated on
PAD_BLOG_SCREENSHOTS=1. One test.describe per blog post; each owns
its post-specific seed and captures into ../../pad-web/static/blog/
<slug>/. First consumer is BLOG-1007 (Conventions and Playbooks);
subsequent posts add a describe block per shot.
* web/e2e/screenshots.spec.ts — refactored to import seedRealisticContent
from the shared lib. No behavior change; PAD_SCREENSHOTS=1 README
capture still passes.
Companion publish helper lives in pad-web at scripts/blog-publish.mjs.
Capture command:
make build-go && cd web && PAD_BLOG_SCREENSHOTS=1 \
npx playwright test blog-screenshots --project=desktop-chromium
Refs TASK-1031, unblocks BLOG-1022 / BLOG-1004 / BLOG-1003 backfill
which all want screenshots.
|
||
|
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.
|