mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
553a39f09bc22aecfb15fdce45f533bc92ffa2b4
527 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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.
|
||
|
xarmian
|
48776a3967 |
feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372)
* feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951)
Third of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. Mounts the three flow-driving HTTP endpoints over the
fosite-backed server constructed in sub-PR B, replaces the
TASK-950 501 stub with the real RFC 8414 discovery doc, and ships
an inline-HTML consent stub as a TASK-952 placeholder so the
auth-code flow runs end-to-end.
What lands:
- internal/server/handlers_oauth.go (744 LoC)
- POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite.
Public clients only (token_endpoint_auth_method=none rejected
for any other value), authorization_code + refresh_token
grants only, code response type only. Validates redirect_uris
(absolute, no fragment, https or loopback-http or custom-
scheme like claude://, blocks file:/javascript:/data:/vbscript:).
- GET /oauth/authorize: starts auth-code flow. fosite validates
request shape (PKCE-S256 required, audience matched, redirect
exact-match). If user has session → renders inline consent
stub. If not → 302 to /login?redirect=<self> (TASK-998's
plumbing in pad-cloud honors the redirect=).
- POST /oauth/authorize/decide: processes consent decision.
Form-bound CSRF token (the existing __Host-pad_csrf cookie,
read from a hidden form field instead of header). Approve →
fosite NewAuthorizeResponse → 303 to client.redirect_uri
with code. Deny → fosite WriteAuthorizeError(access_denied).
- POST /oauth/token: code + refresh exchange. fosite verifies
PKCE verifier (S256-required) + RFC 8707 audience. Returns
{access_token, token_type, expires_in, refresh_token, scope}.
RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS
issues on authorize-code grant.
- Inline consent stub: minimal HTML form with Approve/Deny,
auto-grants every requested scope (TASK-952's UI replaces
with workspace allow-list selection per TASK-953).
- internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub
→ handleOAuthAuthorizationServer. Returns RFC 8414 metadata
with all six endpoint URLs (revoke + introspect URLs sub-PR D
fills with handlers; the URLs are stable now), advertised
scopes, S256-only code_challenge_methods,
resource_indicators_supported=true, authorization_response_iss_parameter_supported=true.
- internal/server/server.go: Server.oauthServer field +
SetOAuthServer + registerOAuthRoutes called from setupRouter
inside an r.Group with requireCloudMode + SessionAuth (so
/authorize can detect the logged-in user via __Host-pad_session;
SessionAuth falls through gracefully when no cookie).
- cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using
cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as
AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL
being set (the OAuth surface needs a canonical audience to
bind tokens to).
CSRF posture: middleware_csrf.go runs only on /api/* paths so
/oauth/* is naturally exempt. The consent decision endpoint
adds its own form-token check (validateConsentCSRFToken) using
the same __Host-pad_csrf cookie the SPA uses, just with the
token in a hidden form field rather than a header. Same security
model, different transport.
Tests (12, all passing):
- TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins
RFC 8414 metadata fields including S256-only PKCE +
resource_indicators_supported.
- DCR (5): happy path; missing redirect_uris; bad redirect-URI
shapes (relative, non-loopback http, fragment, javascript:);
non-public client auth method rejected; unknown grant type
rejected; not mounted outside cloud mode.
- /authorize (3): redirects to /login when no session;
renders consent stub when logged in; rejects audience
mismatch via fosite's audienceMatchingStrategy.
- /authorize/decide (2): rejects missing csrf_token; deny
produces access_denied redirect.
- Full PKCE flow: end-to-end /authorize/decide (approve) →
/token with code_verifier → 200 with access+refresh tokens.
- /token: rejects missing PKCE verifier.
Replaces the 501 stub assertion in TestMCP_AuthServerStub with
TestMCP_AuthServerMetadata_Mounted (just confirms 200; full
shape lives in the OAuth-handler test).
Out of scope:
- /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026)
- MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027)
- Real consent UI with workspace allow-list (TASK-952)
* fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1)
Two findings from PR #372 round 1:
1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT)
send `resource=` not `audience=`. fosite v0.49 reads only
`audience` from the form, so audienceMatchingStrategy was hit
with an empty needle and rejected every real-world authorize /
token request. Tests masked the gap by sending both keys.
Fix: translateResourceToAudience() copies r.Form["resource"]
into r.Form["audience"] before each handler invokes fosite.
Idempotent — if both keys are present, audience wins (test
harness sends both for belt-and-suspenders). Applied at
/authorize, /authorize/decide, and /token entry points.
Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY
resource= (no audience=) and asserts the request reaches the
consent stub. Without the translation it 303s with
invalid_request.
2. P2: /.well-known/oauth-authorization-server advertised
/oauth/revoke + /oauth/introspect endpoints that don't exist
yet (sub-PR D wires them). Real clients dialing those URLs
would get 404. RFC 8414 §2 lists revocation_endpoint +
introspection_endpoint as OPTIONAL, so omitting until the
handlers ship is spec-compliant + honest.
Fix: drop revocation_endpoint, introspection_endpoint, and
their *_endpoint_auth_methods_supported counterparts from
authServerMetadata. Sub-PR D's PR description includes
"populate these here" as a follow-up.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
asserts the four fields are absent.
* fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2)
Two findings from PR #372 round 2:
1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop /
Cursor self-register without prior auth) but had no rate limit.
An attacker could flood the oauth_clients table indefinitely.
Fix: extend RateLimit middleware to gate /oauth/register at
the same 5/hour/IP rate the existing /api/v1/auth/register
uses (RateLimiters.Register, burst 5). Added the OAuth route
group to the s.RateLimit middleware chain so the new path
actually runs through the limiter.
Other /oauth/* endpoints aren't rate-limited here: /authorize
rides session cookies (cheap to abuse but ineffective without
a logged-in user), /token is PKCE-bound to a stored code
(single-use), /authorize/decide is form-bound. Explicit per-
endpoint /oauth/* limits arrive with TASK-959.
Test TestOAuth_Register_RateLimited fires 5 requests
successfully, asserts the 6th returns 429.
2. P2: Discovery doc advertised
authorization_response_iss_parameter_supported=true, but the
/authorize success path delegates to fosite v0.49 which doesn't
add iss=<issuer> to the redirect. RFC 9207-aware clients seeing
the flag would treat the missing parameter as a protocol
violation.
Fix: drop the field from authServerMetadata. RFC 8414 §2
marks it OPTIONAL — omission is spec-compliant. We'll add
the parameter (+ post-processing of fosite's response) in a
future PR if a real client requires it; today's MCP clients
(Claude Desktop, Cursor, ChatGPT) don't.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
extended to cover the field.
* fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3)
Codex round 3 caught: /.well-known/oauth-authorization-server lives
in the MCP route group (registerMCPRoutes), while the /oauth/{
register,authorize,token} handlers live in the OAuth route group
(registerOAuthRoutes, gated on s.oauthServer != nil). A cloud
deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted
but NOT OAuth — the discovery doc would 200 with /oauth/* URLs
that 404. Worse for clients than no document at all.
Fix: handleOAuthAuthorizationServer now also nil-checks
s.oauthServer; on nil it returns 503 with config_error, matching
the existing fail-loud branch for when the issuer URL isn't
configured. Ops detect the misconfiguration immediately rather
than fielding "OAuth registration is failing with 404" tickets.
Test:
- TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled
builds a Server with SetCloudMode + SetMCPTransport (so the
MCP route group mounts) but NOT SetOAuthServer; asserts the
endpoint returns 503 with config_error.
- TestMCP_AuthServerMetadata_Mounted renamed →
TestMCP_AuthServerMetadata_MountedAndGated to reflect the new
behavior under mcpEnabledTestServer (which doesn't wire OAuth).
The full 200 happy path lives in
TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses
oauthEnabledTestServer).
* fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known
* fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485
CI govulncheck rejected the build: fosite v0.49.0 transitively
pulls github.com/go-jose/go-jose/v3@v3.0.3 which has
GO-2025-3485 (DoS in JWS parsing). Affected call site:
internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls
fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned.
Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory).
go mod tidy auto-bumped dependent indirect deps too.
Verified locally:
govulncheck ./... → "No vulnerabilities found"
go test ./... → all green
go build ./... → clean
|
||
|
xarmian
|
f6eeee4f81 |
feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951) (#371)
* feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951)
Second of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. Wires fosite v0.49.0 over the storage layer from sub-PR A.
No HTTP routes yet — sub-PR C mounts /authorize, /token, /register;
sub-PR D mounts /revoke and /introspect.
What lands:
- internal/oauth/session.go — pad's *Session embedding fosite.DefaultSession
with typed UserID() accessor + Clone override returning *Session
(so handler-side type-assertions don't lose the concrete type
during refresh-token rotation).
- internal/oauth/storage.go — Storage adapter satisfying:
fosite.ClientManager
handler/oauth2.AuthorizeCodeStorage
handler/oauth2.AccessTokenStorage
handler/oauth2.RefreshTokenStorage
handler/oauth2.TokenRevocationStorage
handler/pkce.PKCERequestStorage
Compile-time guards in server.go assert each interface remains
satisfied. Translation: fosite.Requester ⇄ models.OAuthRequest
via JSON-encoded session_data + URL-encoded form. Sentinel errors
from sub-PR A map to fosite.ErrNotFound /
ErrInvalidatedAuthorizeCode / ErrInactiveToken.
- internal/oauth/audience.go — RFC 8707 custom AudienceMatchingStrategy.
fosite has no native RFC 8707; we close over a canonical audience
and reject any request that doesn't carry exactly that resource.
Belt-and-suspenders haystack check defends against fixtures /
migrations that register a client without setting Audience.
Plus ValidateAudienceParam (HTTP-handler entry helper) and
audienceForNewClient (DCR seed for sub-PR C).
- internal/oauth/server.go — NewServer(Config) → *Server returning
fosite.OAuth2Provider configured for:
- PKCE-S256 required (EnforcePKCE + EnablePKCEPlainChallengeMethod=false)
- Opaque HMAC tokens (compose.NewOAuth2HMACStrategy)
- Refresh rotation with grant-family revocation (sub-PR A round-2 fix)
- Audience binding via the custom strategy
Sensible default lifespans (1h access, 30d refresh, 15m authcode);
overridable via Config. Excluded by design: client-credentials,
implicit, ROPC (deprecated in OAuth 2.1), OpenID factories
(we're not OIDC), PAR (not needed for v1).
- go.mod — github.com/ory/fosite pinned at v0.49.0 (direct dep).
Tests (20):
- NewServer required-field validation (3) + default-lifespan path
- audienceMatchingStrategy: empty needle, mismatch, client-without-canonical,
canonical-only happy path, multi-audience rejection, no-canonical=ServerError
- ValidateAudienceParam (5 sub-cases) + audienceForNewClient
- Session: clone returns concrete *Session (not *DefaultSession);
nil-safe accessors
- Storage adapter: auth-code round-trip + invalidated-code error,
GetClient not-found mapping, access-token-inactive mapping,
rotation-revokes-entire-grant (end-to-end), PKCE round-trip,
requester-to-OAuthRequest session encoding + missing-client guard
Out of scope (subsequent sub-PRs):
- HTTP route handlers + DCR endpoint (sub-PR C / TASK-1025)
- /revoke + /introspect endpoints (sub-PR D / TASK-1026)
- MCPBearerAuth OAuth introspection branch (sub-PR E / TASK-1027)
* fix(oauth): inject canonical audience into hydrated clients per Codex review (round 1)
Codex round 1 caught a P1 in the storage adapter: modelClientToFosite
returned fosite.DefaultClient.Audience=nil for every persisted
client, but audienceMatchingStrategy's haystack-side check requires
client.GetAudience() to contain the canonical audience. Net result:
every authorize / token / refresh flow would fail with invalid_request
once the strategy ran, regardless of how the client was registered.
Fix: thread the canonical audience through Storage. NewStorage now
takes a canonicalAudience string; modelClientToFosite (now a method
on Storage) injects [canonicalAudience] into the hydrated client's
Audience field. The audience isn't persisted as a column —
single-resource AS for v1 (PLAN-943) means every client implicitly
allows the same audience, so storing what we'd always set to the
same value is pure write amplification.
Threading:
cfg.AllowedAudience → NewServer → NewStorage(store, audience)
└─ Storage.canonicalAudience
└─ modelClientToFosite injects
Misconfigured Storage (empty canonicalAudience — caught earlier by
NewServer's required-field check, but tests pin the fail-loud branch
in case Storage is ever constructed directly): produces clients
with Audience=nil so audienceMatchingStrategy rejects every request
with ServerError, surfacing the misconfiguration fast rather than
silently issuing wide-open tokens.
Tests:
- TestStorage_GetClient_InjectsCanonicalAudience — pins the
injection contract; without the fix this fails.
- TestStorage_NewStorage_EmptyCanonicalLeavesAudienceNil — pins the
fail-loud branch for misconfigured Storage.
- 6 existing tests updated to pass canonical audience to NewStorage
(mechanical sed update; behaviour unchanged).
* fix(oauth): hydrate request payload on inactive token Get*Session per Codex review (round 2)
Codex round 2 caught a HIGH-severity gap: GetRefreshTokenSession
returned (nil, fosite.ErrInactiveToken) for revoked rows, but
fosite's handleRefreshTokenReuse (flow_refresh.go:178-204) derefs
req.GetID() to drive the family revocation that's the OAuth 2.1
BCP §4.14 replay-detection rule. Returning nil nil-derefs that
flow and defeats replay detection — the very thing rotation exists
to enable.
Fix: hydrate the stored row even on the inactive path and return
(req, fosite.ErrInactiveToken). Mirrors the pattern already used
by GetAuthorizeCodeSession's invalidated-code branch. If
hydration itself fails (client deleted between issuance and use),
return the underlying error rather than masking it — replay
detection loses but the failure is observable.
Symmetric fix applied to GetAccessTokenSession even though
no fosite caller currently derefs on inactive there. Defense in
depth + uniform contract makes the adapter resilient to future
fosite changes (e.g. an introspector that wants req.GetID() for
audit-log enrichment).
Tests:
- TestStorage_GetRefreshTokenSession_InactiveReturnsPayload —
pins the refresh-side contract; without the fix this fails
on the nil-check.
- TestStorage_GetAccessTokenSession_InactiveReturnsPayload —
same pattern for access tokens.
* fix(oauth): set RefreshTokenScopes=[] so authorize-code grants issue refresh per Codex review (round 3)
Codex round 3 caught a P1: fosite defaults
Config.RefreshTokenScopes to ["offline", "offline_access"]. fosite
only mints refresh tokens when one of the listed scopes is granted.
PLAN-943's scope vocabulary is pad:read / pad:write / pad:admin —
no "offline" scope — so the default silently disabled refresh
issuance for every Pad grant, defeating the entire refresh-rotation +
family-revocation machinery this PR adds.
Fix: explicitly set RefreshTokenScopes: []string{} in NewServer's
fosite.Config. fosite reads the empty slice as "issue refresh on
every authorize-code grant whose client allows the refresh_token
grant type, no scope predicate" — matches fosite's own tests
(flow_authorize_code_token_test.go:129).
Pin: TestNewServer_RefreshTokenScopesIsEmpty documents the decision
+ smoke-checks that the constructor still returns a usable provider.
The actual "refresh issued on authorize-code grant" assertion lands
in sub-PR C's /token endpoint test — that's where fosite's
flow_authorize_code_token.go reads the field.
* fix(oauth): bump go.opentelemetry.io/otel{,/sdk} to v1.40.0 to resolve GO-2026-4394
CI govulncheck job rejected the build: fosite v0.49.0 transitively
pulls in go.opentelemetry.io/otel/sdk@v1.21.0 which has known
vulnerability GO-2026-4394 (Arbitrary Code Execution via PATH
Hijacking in go.opentelemetry.io/otel/sdk). Affected
init-time call sites:
internal/oauth/audience.go:8 → fosite.init → otel resource.init
internal/server/middleware_ratelimit.go:80 → sync.Once.Do → resource.Default
internal/cli/client.go:415,794,798 → otelhttp.* → trace.*
Fix: bump otel core + sdk + metric + trace to v1.40.0 (the fixed
version per GO-2026-4394's advisory). go mod tidy also pulled in
go.opentelemetry.io/auto/sdk@v1.2.1 as a new transitive.
Verified locally:
govulncheck ./... → "No vulnerabilities found"
go test ./... → all green
go build ./... → clean
|
||
|
xarmian
|
2a00775481 |
feat(oauth): schema + storage layer (TASK-1023, sub-PR A of TASK-951) (#370)
* feat(oauth): schema + storage layer for OAuth 2.1 server (TASK-1023, sub-PR A of TASK-951)
First of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. This one is foundation only — no HTTP exposure, no fosite
import, no public surface change.
Schema (5 tables, parallel SQLite + Postgres migrations):
- oauth_clients — RFC 7591 Dynamic Client Registration; public clients only for v1
- oauth_authorization_codes — short-lived codes for the auth-code grant
- oauth_access_tokens — opaque HMAC; subject denormalized for fast user-bound queries
- oauth_refresh_tokens — same shape; access_token_signature link + request_id chain
- oauth_pkce_requests — PKCE session keyed by auth-code signature
Storage layer (internal/store/oauth.go):
- 12 public methods covering fosite's ClientManager + CoreStorage +
PKCERequestStorage + TokenRevocationStorage interface shapes,
using pad-internal types so the package stays fosite-free.
- Three sentinel errors (ErrOAuthNotFound, ErrOAuthInvalidatedCode,
ErrOAuthInactiveToken) that sub-PR B's adapter maps to the
matching fosite errors.
- request_id IS the chain identifier (fosite preserves it across
rotations — handler/oauth2/flow_refresh.go:86), so family
revocation is a single indexed UPDATE rather than a separate
chain_id column.
14 tests covering: client CRUD + idempotent delete + empty-slice
normalization, auth-code create/get/invalidate (including the
"return payload alongside ErrInvalidatedCode" contract fosite
relies on for family revocation), access-token CRUD + delete,
refresh CRUD + RotateRefreshToken (single-row flip), refresh-token
family revocation (entire chain via request_id, leaves other
chains untouched), access-token family revocation, PKCE CRUD, and
required-field validation.
Both backends share test bodies via testStore(t); set
PAD_TEST_POSTGRES_URL=... to run the same suite against Postgres.
Out of scope for this PR (subsequent sub-PRs):
- fosite import + adapter (sub-PR B / TASK-1024)
- DCR + authorize + token endpoints (sub-PR C / TASK-1025)
- revoke + introspect endpoints (sub-PR D / TASK-1026)
- MCPBearerAuth OAuth integration (sub-PR E / TASK-1027)
* fix(oauth): always insert active=true; drop broken zero-value Active override per Codex review (round 1)
Codex round 1 caught a P1 in insertOAuthRequestRow:
active := defaultActive
if req.Active != defaultActive {
active = req.Active // <- zero-value collides
}
When defaultActive=true and req.Active=false (the zero value), this
branch fires and the row is stored with active=FALSE — silently
producing immediately-revoked tokens. Any sub-PR B adapter that
built an OAuthRequest without explicitly setting Active=true would
ship broken.
Fix: hardcode active=TRUE on insert. Drop the defaultActive
parameter (it's always true for the three flagged tables; PKCE
has no active column). Pre-seeding inactive isn't a supported flow
— fosite never does it, and tests that need a revoked row do
Create + Invalidate / Rotate / RevokeFamily as a two-step.
Regression test TestOAuth_Insert_AlwaysActive constructs an
OAuthRequest with zero-value Active and asserts the row is
readable as active for all three table types (codes, access,
refresh). Without the fix the test fails on the first GetAccessToken
call with ErrOAuthInactiveToken.
* fix(oauth): RotateRefreshToken revokes both refresh + access families per Codex review (round 2)
Codex round 2 caught: my RotateRefreshToken only marked the named
refresh row inactive, but fosite's reference MemoryStore.RotateRefreshToken
(storage/memory.go:497-504) revokes BOTH the refresh family AND the
access family for the grant's request_id. Without this, every access
token issued before a refresh remained active until TTL — defeating
the rotation's invalidation contract.
Fix: RotateRefreshToken now delegates to RevokeRefreshTokenFamily +
RevokeAccessTokenFamily (both already existed). The signatureToRotate
parameter becomes vestigial — fosite passes it but the family revoke
catches every chain member regardless of which row triggered the
rotation. The new pair fosite immediately issues via
CreateAccessTokenSession + CreateRefreshTokenSession inherits the
same request_id (flow_refresh.go:86) and lands active=TRUE per the
round-1 hardcode, so the net post-rotation state is "all old rows
in this grant inactive, the new pair active."
Test rewrite: TestOAuth_RotateRefreshToken_FlipsActiveOnSingleRow
asserted the OPPOSITE behavior (only one row touched) — that was
the original bug. Replaced with TestOAuth_RotateRefreshToken_RevokesEntireGrant
which seeds a refresh + access pair in the same chain, plus a
distinct unrelated grant, then asserts after rotation:
- old refresh + old access both inactive
- unrelated grant untouched (request_id-scoped)
* fix(oauth): DeleteOAuthClient cascades dependent rows in a tx per Codex review (round 3)
Round 3 finding: DeleteOAuthClient errored with FK constraint
violation for any client that had ever issued a grant. The
migrations declare client_id FKs without ON DELETE CASCADE — by
design, so a stray DELETE FROM oauth_clients elsewhere fails
loudly rather than silently nuking grants — but that meant the
"officially supported" delete path was unusable.
Fix: DeleteOAuthClient now runs five sequential DELETEs inside a
single transaction:
1. oauth_pkce_requests
2. oauth_refresh_tokens
3. oauth_access_tokens
4. oauth_authorization_codes
5. oauth_clients
Order matters (children before parent) because the FKs aren't
cascading. The tx makes it atomic — if any step fails, nothing's
deleted, so we never leave a half-deleted client. Idempotent
because every WHERE matches nothing on a non-existent client.
Test TestOAuth_DeleteOAuthClient_CascadesDependentRows seeds a row
in each of the four dependent tables, deletes the client, and
asserts ErrOAuthNotFound on every dependent row + the client itself.
Without the fix this fails on the first DELETE FROM oauth_clients
with an FK constraint violation.
* fix(oauth): SELECT FOR UPDATE row lock in DeleteOAuthClient on Postgres per Codex review (round 4)
Codex round 4 caught a Postgres race in DeleteOAuthClient: the
five-DELETE cascade is atomic, but between the child-row deletes
and the parent delete, a concurrent fosite handler can insert a
fresh grant/token referencing the same client_id. The parent
DELETE then fails with an FK violation and the whole tx rolls
back — the cascade is correct, but unreliable under concurrent
OAuth issuance.
Fix: take SELECT id FROM oauth_clients WHERE id = ? FOR UPDATE
as the very first statement in the tx (Postgres only). The
exclusive row-level lock blocks any concurrent statement that
tries to read the client row — which fosite does on FK resolution
during grant/token inserts — until our tx commits.
Skipped on SQLite because:
(a) BEGIN IMMEDIATE serializes the entire write workload globally
(DSN configures _txlock=immediate per store.go), so the race
doesn't exist.
(b) FOR UPDATE syntax isn't reliably accepted across SQLite
drivers.
ErrNoRows on the lock query is treated as "client doesn't exist
yet" — the subsequent DELETEs match nothing and the call remains
idempotent. Tests still pass on the SQLite path; the Postgres
path's race fix will be exercised by CI's PAD_TEST_POSTGRES_URL
runs and any future concurrency test we add.
|
||
|
xarmian
|
521853e0a1 |
feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369)
* feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied. |
||
|
xarmian
|
12bd442711 |
feat(auth): contextual OAuth-intent banner on /login + /register (TASK-1001) (#368)
Add a small informational banner that renders when a user lands on
/login or /register mid-OAuth-flow (i.e. ?redirect=/oauth/authorize?...).
Tells them what they're in the middle of so the form doesn't read as a
non sequitur for first-touch users coming from the marketing site's
"Connect to Claude" CTA.
The banner is generic-only for now — once TASK-951 ships the OAuth
authorization server and the /api/v1/oauth/clients/{id}/public-info
endpoint, a follow-up will parse client_id from the inner query
string and substitute a friendly name ("connect Claude Desktop"
instead of "connect an AI agent"). Component contract is shaped to
allow that extension without consumer changes.
Detection is heuristic: redirectTarget.startsWith('/oauth/authorize').
False positives only mean a slightly more specific banner; false
negatives leave the user with the same UX they had before.
Mode prop drives the verb: signin ("signing in") on /login, signup
("creating an account") on /register.
Parent: PLAN-943.
|
||
|
xarmian
|
a9ad767a45 |
feat(auth): extract <AuthOAuthButtons> + render on /register (TASK-1000) (#367)
* feat(auth): extract <AuthOAuthButtons> + render on /register (TASK-1000) Lift the cloud-mode SSO block (Continue with GitHub / Google) out of /login into a shared AuthOAuthButtons.svelte component, render it on both /login and /register, and extract redirect= validation + query-string helpers into $lib/auth/redirect so both pages compose the same encoding. Why: the marketing-site "Sign up to connect Claude" CTA lands new users on /register, which had no SSO buttons — first-touch users fell off the 30-second-onboard path. With this PR, /register exposes the same one-click SSO buttons as /login and preserves the ?redirect= query through the click so completing SSO returns to the original destination (e.g. /oauth/authorize?... once TASK-998 ships pad-cloud's redirect= honoring). Behavior changes: - /register reads the same `redirect` query param /login does and honors it on goto() after password registration. - /register populates the "Last used" pill from localStorage so returning users see the visual lift on their preferred provider. - /login is byte-identical: the inline SSO block is replaced with the component, the inline redirect helpers replaced with helper imports, and the now-unused CSS rules removed. Out of scope (separate tasks): pad-cloud's OAuth callback honoring ?redirect= (TASK-998), the OAuth-intent banner (TASK-1001). Parent: PLAN-943. * fix(auth): preserve redirect= across login↔register cross-links per Codex review (round 1) The "Don't have an account? Sign up" link on /login and the "Already have an account? Sign in" link on /register were dropping the current `redirect` query, breaking the OAuth/deep-link flow when a user mid-/oauth/authorize bounced between the two pages. Now both links append the encoded redirect target via redirectQueryFragment. Parent: PLAN-943, TASK-1000. |
||
|
xarmian
|
48bbe7453b |
fix(dashboard): suggested_next surfaces in-progress + filters blocked items (BUG-990) (#366)
Pre-fix algorithm only considered status == "open" child items in active plans. Two consequences agents flagged in BUG-987 / BUG-990: 1. In-progress items never appeared. The most likely "what should I work on next" answer is "the work the user is already on" — pre-fix the engine returned [] when nothing was open AND a task was actively in-progress. 2. Blocked items appeared. Suggesting work that has unresolved blockers wastes the user's time when they go to start it. Algorithm changes (internal/server/handlers_dashboard.go): - Include both `open` and active-status (in-progress / fixing / exploring / etc., via existing isActiveStatus helper) child items. - Filter out items with at least one active "blocks" link from a non-done blocker. Mirrors the attention-section logic, factored into a new itemBlockedByActive helper. - Sort: in-progress first (always wins over open, regardless of priority), then by priority rank within each bucket. Pinned in test expectations. - Reason text distinguishes "In-progress task..." vs "Open task..." so agents see why an item was suggested. Tests: - TestDashboardSuggestedNext expectations updated for new ordering (in-progress wins over open). Test now covers the in-progress surfacing path the bug specifically wanted. - New TestDashboardSuggestedNext_FiltersBlockedItems exercises the blocker-filter — explicitly creates a blocks link and confirms the blocked task is suppressed even at critical priority. Out of scope: high-priority orphan suggestions (items not in any active plan). The bug item flagged this as a stretch goal; this PR sticks to the active-plan-children scope of the existing algorithm. Adding orphans would need a sort/relevance model since "everything high priority" can be a long list. Parent: BUG-990.v0.1.0-rc.6 |
||
|
xarmian
|
9657051e43 |
fix(mcp): dedup implementation_notes / decision_log from fields blob (BUG-992) (#365)
Item responses carry implementation_notes and decision_log in TWO places: 1. Top-level arrays on the item (item.ImplementationNotes, item.DecisionLog) — populated by hydrateItemComputedMetadata. 2. Inside the stringified `fields` blob — written there by AppendImplementationNote / AppendDecisionLogEntry at write time. This duplicate forces agents to dedup or pick a source; the bug report's recommendation was to keep the top-level arrays as the canonical shape and drop the embed. Path-consistent with BUG-991 path A (also MCP-only normalization): extend the boundary normalizer in packageJSONResult so that AFTER fields is parsed (BUG-991), implementation_notes and decision_log keys are dropped from the parsed fields object. The top-level arrays continue to surface unchanged. Server / web / CLI keep their existing behavior — fields still carries the embed at rest, hydration still extracts to top-level. The boundary fix is the cheap clean-up; a write-side migration (stop persisting into the fields blob, plus a one-shot data migration to clean existing items) is the architecturally proper fix and remains tracked in BUG-992's notes. Tests: - internal/mcp/bug992_test.go: stripDuplicatedFieldsKeys helper (strips both keys, no-op when absent, defensive on non-object inputs) + end-to-end packageJSONResult cases for single-item and array-style responses. Parent: BUG-992. |
||
|
xarmian
|
708897dd0c |
fix(mcp): parse fields/tags at MCP boundary so agents see native shapes (BUG-991 path A) (#364)
Item responses carry `fields` and `tags` as JSON-stringified strings
because the underlying SQLite columns store them that way. For agents
going through MCP this means a double-encode every read — they have
to JSON.parse the field's string value before doing anything useful.
Path A (this PR): normalize at the MCP boundary. Recursively walk
parsed JSON in packageJSONResult, find string-typed `fields` and
`tags` properties, parse the embedded JSON, substitute the native
shape. Server / web / CLI keep their existing stringified contract;
agents see clean JSON.
The walk handles every common item shape:
- Single-item responses (top-level item)
- Item arrays (item list, dashboard.active_items, comment lists)
- Nested items (dashboard.recent_activity[].item, parent_*)
Conservative parse: only strings starting with `{` or `[` and
successfully parseable as JSON get substituted. Hand-written values
that happen to share a key name (e.g. a `fields` description text)
pass through untouched. Malformed JSON also passes through as the
original string rather than dropping the value.
Text fallback (content[0].text on the MCP result) preserves the
original CLI body verbatim. Older clients that read the text content
keep seeing the same shape — the structured wire is the agent
upgrade path; text is the back-compat path. Same pattern as BUG-985's
{items: [...]} array wrap.
Path B (full migration of models.Item.Fields from string to
map[string]any across server/store/web/CLI) is tracked in BUG-991's
notes — bigger surgery, deferred.
Tests:
- internal/mcp/bug991_test.go: single-item, item-array, nested-item,
primitives-pass-through, malformed-stays-string, plus end-to-end
packageJSONResult cases proving structured + text branches both
work.
- Existing dispatch_http_advanced_test.go and dispatch_http_project_test.go
updated: tests that previously did `json.Unmarshal([]byte(fieldsStr))`
now use a small itemFieldsAsMap helper that reads the parsed map
directly. Cleaner, and a clear error message if normalization
regresses.
Parent: BUG-991.
|
||
|
xarmian
|
eb1931d747 |
feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989) (#363)
* feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989)
Six pad item subcommands previously returned plain-text confirmations
when called with --format json (and therefore via MCP) — e.g.
"Added implementation note to TASK-8 ...\n", "⭐ Starred TASK-7 ...\n".
Agents had to scrape the text for refs / IDs / status. Now each
emits a structured envelope on the JSON branch, matching the shape
the bug report's recommendations specified.
Per-command shapes:
- pad item note --format json:
{ ref, title, note: { id, summary, details, created_at, created_by } }
- pad item decide --format json:
{ ref, title, decision: { id, decision, rationale, created_at, created_by } }
- pad item star --format json:
{ ref, title, starred: true }
- pad item unstar --format json:
{ ref, title, starred: false }
- pad item delete --format json:
{ ref, title, status: "archived" }
- pad item bulk-update --format json:
{ updated: [{ref, applied: {status?, priority?}}], failed: [{ref, error}], total }
Implementation notes:
- For note/decide: capture the entry locally before persisting so the
JSON branch can echo the freshly-created ID + timestamp without
re-fetching the item.
- For bulk-update: collect per-item outcomes in two slices (updated,
failed) so the response carries which refs succeeded with which
applied changes vs which failed with what error. Suppresses the
human-readable per-line ✓/✗ output when --format json (single
payload at the end is the agent's source of truth).
- Human-readable (default) output paths unchanged for all six.
The text fallback that ExecDispatcher passes back through
packageJSONResult also keeps showing the same structured JSON now
since the CLI emits JSON directly when --format json is set —
matches BUG-985's wrap-arrays-as-{items:[...]} pattern for list
shapes, and rounds out the v0.2 catalog's structured-everywhere
contract.
Live verified all six locally (CLI direct + MCP transport):
pad item note TASK-994 "..." --format json → {note: {...}, ref, title}
pad item star TASK-994 --format json → {ref, starred: true, title}
pad item bulk-update --priority medium TASK-994 --format json
→ {updated: [...], failed, total}
pad item delete TASK-994 --format json → {ref, title, status: "archived"}
(note/decide/star/unstar/decide via MCP confirmed dict-shaped
structuredContent with the expected keys.)
Parent: BUG-989.
* fix(cli): delete JSON envelope uses `archived: true` per Codex review (round 1)
Codex finding: `pad item delete --format json` was emitting
`"status": "archived"`, but the store's delete path only sets
`deleted_at` — the item's persisted `status` field is untouched. So
the envelope's status would mislead agents into treating the item's
status as archived, which breaks if the item is later restored (its
original status field is still there).
Fix: rename to `"archived": true` — unambiguous about what actually
happened (soft-delete marker set) and doesn't collide with the
persisted status semantics.
The other five JSON shapes weren't affected.
|
||
|
xarmian
|
55d3a078a8 |
fix(mcp): standup CLI ref + classifier polish for BUG-987 round 2 (#362)
Round-2 hotfix on top of PR #361 (which shipped to v0.1.0-rc.4). Claude Desktop's re-review of rc.4 surfaced two fixes that didn't fully land: - Bug 8 (round 1 went to wrong layer). My HTTPHandlerDispatcher fix populated ref on standup blockers, but Claude Desktop's path is ExecDispatcher → CLI subprocess → standupCmd, which has its own JSON composition struct. That struct's Attention + SuggestedNext anonymous types didn't even define ItemRef as a parseable field. Now both define `item_ref` and the JSON-emit loops set Ref from it. Verified live: blockers now carry refs (TASK-X), not empty strings. - Bug 11 part 2. Round 1 stripped the cobra Usage block but two artifacts still leaked: 1. The "pad <verb> failed: <stderr>" prefix on server_error fallback messages. The verb name is the OLD CLI verb (e.g. `pad item block`) which doesn't match the v0.2 catalog actions agents see, and the cmdPath is already implicit from the invoked tool. Drop the prefix; emit the cleaned stderr directly. 2. Self-link / "cannot ..." validation rejections classified as server_error instead of validation_failed. Extended the validation regex with `cannot ` so server-side rejections like "cannot link an item to itself" / "cannot modify archived item" route to ErrValidationFailed. Verified live with a self-link attempt — now returns code=validation_failed, hint="cannot link an item to itself", no prefix. - New stripErrorPrefix helper trims leading `Error:` / `error:` / `ERROR:` from every classified hint+message so the envelope text isn't redundant with the envelope's `code` signal. Bug 13 / Bug 14: my round-1 fixes verified working locally on rc.4 (tested with a fresh Task → convention=None; dashboard by_role shows "Unassigned"/"unassigned" for the bucket). The reviewer's stale results almost certainly reflect a pad server process that wasn't restarted with the rc.4 binary swap. Tests: - TestClassifyExecError_CannotPhrasingClassifiesAsValidation — three "cannot ..." stderr cases must classify validation_failed. - TestClassifyExecError_NoLegacyVerbPrefixInMessage — pins the prefix-strip behaviour on the server_error fallback path. - TestStripErrorPrefix — trim-rule round-trip across casing variations and empty input. Parent: BUG-987.v0.1.0-rc.5 |
||
|
xarmian
|
0f05012169 |
fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14) (#361)
* fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14)
Hotfix follow-up to v0.1.0-rc.3's Claude Desktop dogfood. Six
surgical fixes; bigger items (5, 7, 9, 10) deferred to separate
tasks.
- Bug 6: `pad project next --format json` was emitting the entire
dashboard, indistinguishable from `pad project dashboard --format
json`. Now slices to suggested_next only. cmd/pad/main.go.
- Bug 8: standup blockers carried empty `ref` strings, blocking
agent linkback to the actually-blocked items. dashboard's
attention[].item_ref is canonical; the standup composer in
internal/mcp/dispatch_http_slice4.go just wasn't propagating it.
Same fix applied to suggested_next entries.
- Bug 11: cobra's auto-emitted "Usage: pad item block ..." help
block leaked into MCP error envelopes via classifyExecError. The
Usage text references OLD CLI verb names (pre-v0.2 catalog) that
agents using the new surface have no business seeing, and bloats
every error response. New stripCobraUsageBlock helper truncates
stderr at the first line-anchored "Usage:" marker before
classification + envelope construction.
- Bug 12: BuildCLIArgs validation errors (missing required arg, type
mismatch) came out of env.Dispatch as bare-text NewToolResultErrorf
results, breaking the structured envelope contract. New helper
validationFailedFromBuildErr wraps them as ErrValidationFailed
envelopes with the field name extracted via regex from the
underlying message.
- Bug 13: every Task / Idea / Plan with a `priority` field got a
phantom `convention: { enforcement: "<priority>" }` surfaced on
its response, because ExtractItemConventionMetadata's legacy
fallback treated `priority` as the Convention enforcement tier
unconditionally. Restructured to track hasConventionShape
separately from hasMetadata — only Convention-specific markers
(structured convention field, trigger, scope, surfaces, commands,
direct enforcement) flip the shape flag. category alone is
insufficient (Ideas / Bugs / Roadmap items legitimately use it).
Final guard returns nil when only category was matched.
- Bug 14: GetRoleBreakdown's unassigned row was emitted with empty
role_name + role_slug, presenting as a "phantom" entry in the
dashboard. Now explicitly labelled "Unassigned" / "unassigned"
while keeping role_id null so it's still distinguishable from a
real role.
Tests:
- internal/mcp/bug987_test.go (new) — stripCobraUsageBlock + classify
+ validation envelope wrapping + env.Dispatch integration.
- internal/models/item_test.go — three cases covering non-Convention
items (Task, Idea, Plan with priority) returning nil metadata, and
one preservation test for legacy Conventions with priority field.
- internal/store/agent_roles_test.go (new) — confirms unassigned row
carries explicit "Unassigned" / "unassigned" labels.
Live verified: pad_project action=next returns just the suggestions
array; pad_item action=create with no fields returns validation_failed
with field=collection; pad_item action=link with self-target returns
without Usage-block leakage.
Deferred to separate items (per BUG-987 triage):
- Bug 5: text vs JSON returns across note, decide, star, unstar,
delete, bulk-update — needs CLI-side handler updates per command.
- Bug 7: suggested_next algorithm — needs to consider in-progress
items, not just open ones; behavior change needs design.
- Bug 9: fields/tags double-stringified — potentially breaking for
web UI/CLI consumers.
- Bug 10: decision_log/notes embedded in fields blob duplicating
top-level arrays — might require data migration.
Parent: BUG-987.
* fix(mcp): HTTP transport equivalence + ordering for BUG-987 per Codex review (round 1)
Two findings from Codex review of PR #361:
1. project.next on HTTP transport still returned the full dashboard.
The route table mapped "project next" directly to /dashboard, so
the CLI fix (slice to suggested_next) didn't reach OAuth-authed
agents going through HTTPHandlerDispatcher. Catalog actions must
produce equivalent shapes on stdio and HTTP — that's the contract
that lets agents be transport-agnostic.
Fix: new dispatchProjectNext method on HTTPHandlerDispatcher that
fetches the dashboard via the existing fetchDashboardJSON helper,
slices to suggested_next[], re-encodes, and runs through
packageJSONResult so it gets the same {items: [...]} wrap as
other list responses.
Also retires the broken route-table entry — replaced with a
comment pointing at the new method so future contributors don't
re-add a passthrough.
Test: TestDispatch_ProjectNext_SlicesToSuggestedNext + the empty-
array case. Asserts dashboard-only top-level fields (summary,
active_items) don't leak into the response — that's the whole
point of project.next being distinct from project.dashboard.
2. ExtractItemConventionMetadata's priority→enforcement legacy
fallback ran BEFORE surfaces/scope/commands had a chance to flip
hasConventionShape, so a Convention with only `{scope, priority}`
would silently drop enforcement.
Fix: move the priority fallback to AFTER all marker checks. Direct
`enforcement` still resolves first; the legacy priority fallback
runs at the bottom once shape detection is complete.
Tests: two new cases covering scope-only and commands-only legacy
Conventions — both must resolve enforcement via the priority
fallback.
Parent: BUG-987.
v0.1.0-rc.4
|
||
|
xarmian
|
4cf0c297e4 |
fix(mcp): explicit workspace param + wrap list responses (BUG-985) (#360)
Two regressions surfaced by Claude Desktop dogfooding v0.1.0-rc.2:
1. Explicit `workspace` parameter silently dropped (bug 1).
`--workspace` is registered as a persistent ROOT flag in cobra
(rootCmd.PersistentFlags), so cmdhelp doesn't include it in any
leaf command's per-command Flags map. BuildCLIArgs's per-flag
iteration only emits flags from cmdInfo.Flags — meaning the
explicit `input["workspace"]` value was never read or emitted.
The post-loop session-fallback fired in some cases, masking the
problem when a session default was set, but agents passing
`workspace=docapp` explicitly got `no_workspace` errors despite
the catalog schema documenting the resolution order as
"explicit > session > .pad.toml".
Fix: after the per-flag loop, check input["workspace"] directly.
Prefer explicit, fall back to sessionWorkspace, otherwise omit
(CLI handles CWD .pad.toml). The old session-only branch
collapses into this combined check.
2. List responses produced top-level array structuredContent, which
MCP host validators (Claude Desktop) reject with "expected:
record" (bug 3). Affected pad_collection list, pad_workspace
list, pad_role list, item deps, item starred, webhook list,
etc. — anywhere the CLI emits a JSON array.
Fix: extract a shared `packageJSONResult` helper that wraps
top-level arrays in `{items: [...]}` for structuredContent. The
text fallback preserves the ORIGINAL JSON so clients reading
text content keep seeing the raw array shape they used to.
ExecDispatcher.Dispatch and packageHTTPResponse both go through
the helper now, so stdio + HTTP transports produce identical
wire shapes.
Bug 2 (claim that only pad_project honors the session default) didn't
reproduce in isolation locally — the symptom was bug 1 manifesting
inconsistently under concurrent JSON-RPC, with pad_project's empty
flag list dodging the loop entirely while flag-bearing commands hit
the dropped-explicit code path. The bug 1 fix resolves both.
Bug 4 (tool_surface_stable: true vs reality) self-resolves once 1-3
land — the surface is now stable as documented.
Tests:
- internal/mcp/bug985_test.go (new) — 10 subtests covering
BuildCLIArgs explicit/session/no-flag-in-cmdinfo cases plus the
packageJSONResult wrap behavior across object/array/non-JSON/
empty-array/malformed/marshal-round-trip scenarios.
- Existing integration tests that asserted the old top-level-array
shape on item.deps / item.starred / item.list / workspace.list /
webhook.list now go through an unwrapItems(t, sc) helper so the
refactor lands in one consistent place.
Live verified: from /tmp (no .pad.toml), all four bug-report
operations now succeed with explicit `workspace=docapp` and return
`dict` structuredContent.
Parent: BUG-985.
v0.1.0-rc.3
|
||
|
xarmian
|
273d75c06e |
docs(mcp): refresh README + SKILL.md for v0.2 surface (TASK-976) (#359)
Updates the in-repo documentation to match what shipped in PLAN-969: - README.md's MCP section now describes the v0.2 catalog (8 tools, resource × action shape) instead of the retired v0.1 verb explosion. Documents both stability constants (CmdhelpVersion 0.1 + ToolSurfaceVersion 0.2) and points consumers at the structured error envelope contract. - skills/pad/SKILL.md gets a one-line callout that the MCP surface is hand-curated and distinct from the CLI verb tree this skill drives. Prevents future "I added a CLI command, why isn't it in MCP?" confusion. - CLAUDE.md was already updated in TASK-981; verified to match. Companion change for getpad.dev/mcp/local lives in ../pad-web. Parent: TASK-976 → PLAN-969.v0.1.0-rc.2 |
||
|
xarmian
|
a928222ace |
feat(mcp): add pad://workspaces top-level resource (TASK-974) (#358)
Promotes the workspace catalog to a static MCP resource so hosts can
prefetch it once at session start instead of forcing a pad_workspace.list
tool call per turn.
URI: pad://workspaces
Shape: JSON array of {slug, name, updated_at, default} entries —
exactly the shape `pad workspace list --format json` emits, so the
resource handler and classifyExecError's available_workspaces side
channel consume the same source of truth.
Implementation:
- internal/mcp/resources.go: add WorkspacesURI constant + readWorkspaces
handler. Registered via AddResource (not AddResourceTemplate) since
the URI is parameter-free and lives in resources/list.
Tests:
- TestReadWorkspaces_DispatchesWorkspaceListJSON: end-to-end resource/read
round-trip through HandleMessage; asserts the fetcher saw the right
CLI args.
- TestReadWorkspaces_RejectsWrongURI: defensive guard against URI/handler
binding drift in future refactors.
- TestReadWorkspaces_PropagatesFetcherError: fetcher errors surface
cleanly to the MCP client instead of being swallowed.
Out of scope (per task description):
- Live updates / resources/subscribe — future enhancement.
- Per-collection resource (pad://workspace/{ws}/collections/{slug}) —
flat list stays for now.
Parent: TASK-974 → PLAN-969.
|