mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-21 01:53:33 +00:00
v0.2.0
289 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
1ff6158468 |
feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) (#415)
* feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101)
Foundation for PLAN-1100 (client-side permission audit). Lands the primitive
that every other task in the plan consumes, with no UI behavior changes.
Server:
- new GET /api/v1/workspaces/{ws}/me — returns role, collection_access,
visible_collection_ids (computed via VisibleCollectionIDs /
GuestVisibleCollectionIDs so it covers system collections, member access,
direct collection grants, and item-grant collections), plus the user's
direct collection_grants and item_grants.
- admins normalize to "owner"; legacy workspace-scoped tokens normalize to
"editor"; non-members with no grants are rejected upstream by
RequireWorkspaceAccess and never reach the handler.
Frontend:
- new $lib/utils/permissions module exporting pure cascade functions:
canEditWorkspace / canViewCollection / canEditCollection /
canViewItem / canEditItem.
- cascade mirrors server's ResolveUserPermission exactly:
owner → item grant → collection grant → membership role + visibility
so item grant beats collection grant beats role even when less permissive
(ItemGrant.view + CollectionGrant.edit on same item → effective view).
- workspaceStore wraps the pure functions with currentMembership state
fetched in setCurrent. New getters: currentRole, currentMembership,
isOwner, canEditWorkspace; new methods: canViewCollection /
canEditCollection / canViewItem / canEditItem.
- WorkspaceMembership type added.
- api.workspaces.me(slug) added.
Refactor:
- settings/+page.svelte, [collection]/+page.svelte,
[collection]/[slug]/+page.svelte: drop open-coded role derivation
(members.find + m.role open-codes), consume workspaceStore.isOwner.
members.list calls remain — still needed for assignee dropdowns / member
rows in settings — only the role-derivation path moves to the store.
Tests:
- server: handlers_me_test.go covers 6 scenarios
(admin, editor with all-access, viewer with collection grant,
restricted member, guest with item grant, non-member with no grants).
- frontend unit tests deferred — web/ has no unit-test runner today.
Pure-function module makes them trivial to add when the runner lands.
Cascade is independently covered by store/permissions_test.go and
store/grants_test.go on the server.
Parent: PLAN-1100.
* fix(workspace): per-item visibility uses strict full-access set + setCurrent race guard per Codex review (round 1)
P1: canViewItem fell back to canViewCollection, which uses the broad nav
set (visible_collection_ids — includes collections containing
item-granted items so they appear in nav). This meant a guest with one
ItemGrant on TASK-5 in Tasks would see canViewItem(any-other-task-in-Tasks)
return true, while the server only allows direct item grants or full
collection grants.
Fix: /me now also returns full_access_collection_ids — the strict set of
collections in which every item is accessible (collection grants +
member_collection_access + system collections; item-grant collections
intentionally excluded). This mirrors guestResourceFilter's fullCollIDs
in handlers. canViewItem and canEditItem now consult full_access_collection_ids
on the membership-fallthrough path, NOT the nav set.
Test added: TestMe_GuestWithItemGrant now asserts the item-grant collection
is in visible_collection_ids (nav) but NOT in full_access_collection_ids
(strict). TestMe_RestrictedMember updated to check both sets.
P2: workspaceStore.setCurrent had no guard against stale async /me responses.
A slow /me for workspace A could clobber a freshly-fetched membership
for workspace B if the user navigated mid-flight, briefly exposing
permission-gated UI for the wrong workspace.
Fix: monotonic membershipSeq counter incremented per setCurrent / create
call. Each /me response only writes back if its captured token still
matches at resolution time. Also clears currentMembership immediately on
setCurrent so helpers don't briefly answer "yes" using the previous
workspace's grants while /me is in flight.
Parent: PLAN-1100. Refs TASK-1101 PR #415.
* fix(workspace): canEditCollection uses strict full-access set per Codex review (round 2)
Same nav-vs-strict bug pattern as round 1's canViewItem fix, but in
canEditCollection. The editor-membership fallback path previously gated
on canViewCollection (broad nav predicate using visible_collection_ids),
which incorrectly returned true for a restricted editor whose only access
to a collection was an item grant. The collection appears in nav (correct)
but the editor must NOT see collection-wide write affordances like "+ New"
because the server rejects collection-level writes there.
Fix: editor membership fallback now requires either collection_access ===
"all" or the collection to be in full_access_collection_ids.
canEditItem already used full_access_collection_ids on its fallback path
(it was added in round 1) — verified unchanged.
Parent: PLAN-1100. Refs TASK-1101 PR #415.
|
||
|
xarmian
|
89e9551ae3 |
test(store): end-to-end onboarding walkthroughs for scrum + product (TASK-1151) (#410)
Mirrors TestOnboardingFlow_FullWalkthrough_Startup (PR #405) for the two newly-seeded software-category templates. Two new test fns, same three-phase shape: Phase 1 — Fresh seed: - Four onboarding seeds land at the right item_numbers + statuses. - Conventions + playbooks present (after the user-facing seeds). - Primary entry starts in its initial status (BACK-1=new for scrum; FEAT-1=proposed for product) — the gate the dashboard banner relies on. Phase 2 — Agent walks user through populating real items: - Primary's status flips out of initial (signaling engagement). - Real workspace activity gets captured in the template's verbs: scrum: a sprint, three real backlog items linked to it, one bug product: a roadmap commitment, three features under it, one user-feedback item from a sales call - Primary flips to terminal (BACK-1 → done; FEAT-1 → shipped) — banner hides on next dashboard refresh. Phase 3 — Idempotency on re-trigger: - User's items remain untouched. - Primary stays at terminal status — re-seed must NOT reset to initial, which would silently re-show the banner. - No duplicate seed items. - Conventions + playbooks counts unchanged. Reuses the helpers added in PR #405 (findItemByTitle, extractStatus, setItemStatus, countItemsInCollection) — no new helpers needed. This is the gate task for PLAN-1146. With this merged, scrum + product now have the same coverage as startup did after PLAN-1131. Parent: PLAN-1146. |
||
|
xarmian
|
abf017c4e7 |
feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409)
The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum
workspaces surface "use pad to get BACK-1", product workspaces surface
"use pad to get FEAT-1", and any future template that ships an
agent-onboarding seed declares its primary ref once and gets the
banner / hint for free.
Mechanism:
1. WorkspaceTemplate gains an OnboardingPrimaryRef string field —
the canonical declaration of "this template's IDEA-1-style
primary entry." Set per template that ships the pattern
(startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1");
left empty for hiring/interviewing/demo where the agent-onboarding
pattern intentionally doesn't apply.
2. Server: handleGetDashboard identifies the seeded primary by
walking allItems looking for item_number=1 + source="template"
+ created_by="system" + collection_slug ∈ {ideas, backlog,
features}. The collection-slug whitelist is what keeps hiring's
REQ-1 (also seeded with item_number=1 + source=template) from
being flagged as an onboarding entry — those are example items,
not agent scripts. The dashboard response gains an
onboarding_seed field with ref/title/slug/collection_slug/status
plus a server-computed `active` boolean (true iff status equals
the schema initial value).
3. CLI: printOnboardingHints accepts the template name, looks up
the primary ref via collections.GetTemplate, and prints the
right "use pad to get X-1" line. Templates without a declared
primary skip the line entirely (so hiring's pad init success
doesn't promise a non-existent BACK-1 / IDEA-1).
4. Web frontend: dashboard reads dashboard.onboarding_seed,
gates the banner on `active=true`, passes ref/slug/collection
to OnboardingIdeaBanner. The component renders the trigger
phrase, copy button, and "Read it first" deep link from those
props — no more hardcoded IDEA-1.
ensureWorkspace's signature gains a returned templateName so init.go
+ main.go can pass it through to printOnboardingHints. The five
existing test call sites updated.
New tests:
internal/collections/templates_test.go
- TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template
OnboardingPrimaryRef values (and the explicit emptiness of
hiring/interviewing/demo).
internal/server/handlers_dashboard_test.go
- TestDashboardOnboardingSeed_StartupTemplate
- TestDashboardOnboardingSeed_ScrumTemplate
- TestDashboardOnboardingSeed_ProductTemplate
- TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed —
hiring's REQ-1 is example data, not an onboarding entry)
- TestDashboardOnboardingSeed_EmptyWorkspace (no template)
Removes the loadIdeaOne race-guard from +page.svelte — the dashboard
poll itself now carries the onboarding_seed.active flag so the banner
state lives entirely in the dashboard response. Drops ~50 lines of
frontend code.
Parent: PLAN-1146.
|
||
|
xarmian
|
8fc0cb3b8b |
feat(collections): seed onboarding items + explicit prefixes for scrum + product templates (TASK-1149) (#408)
* feat(collections): seed onboarding items + add explicit prefixes for scrum + product templates (TASK-1149) Mirrors TASK-1133's pattern (PR #402) for the remaining software-category templates. After this lands: - fresh `pad workspace init --template scrum` → BACK-1 / SPRINT-2 / BUG-3 / DOC-4 - fresh `pad workspace init --template product` → FEAT-1 / FB-2 / ROAD-3 / DOC-4 Each is a first-person note from the workspace owner's future self — agent-invocable via `/pad let's discuss <REF>`, schema-aware terminal verbs ("mark me done" / "completed" / "shipped" / "archived"), no "tutorial" / "lesson" language. Bodies pulled verbatim from DOC-1152 (scrum) and DOC-1153 (product). Precondition fix: explicit Prefix set on five collections so DerivePrefix doesn't yield awkward refs: Backlog BACKL → BACK Sprints SPRIN → SPRINT Features FEATU → FEAT Feedback FEEDB → FB Roadmap Items RI → ROAD Mirrors hiring template's pattern of explicit prefixes on its custom collections. Existing scrum/product workspaces (forward-only fix) keep their derived prefixes — the seeder doesn't migrate. New tests: internal/collections/templates_test.go - TestScrumOnboardingItemsOrderAndShape - TestProductOnboardingItemsOrderAndShape - TestScrumProductTemplatesShipOnboardingSeedItems - TestScrumProductTemplatesUseExplicitFriendlyPrefixes (locks the prefix-fix precondition) internal/store/items_test.go - TestSeedCollectionsFromTemplateScrumRefSequence - TestSeedCollectionsFromTemplateProductRefSequence (Both also assert the prefix lands on each seeded item — drift in templates.go would surface here as a test failure pointing at the PLAN-1146 prefix precondition.) Existing onboarding test (TestSeedCollectionsFromTemplateStartupRefSequence) still passes — startup template untouched. Parent: PLAN-1146. Source content: DOC-1152, DOC-1153. * docs(comments): clarify the post-signup hint is wired in TASK-1150, not this PR (Codex review round 1) Codex flagged that the helper-file + templates.go comments said things like "the post-signup hint will name BACK-1" — which read as "it does today" but actually means "it will once TASK-1150 lands." Until that ships, the dashboard banner and CLI hint still hardcode IDEA-1 from PR #403, so a fresh scrum/product workspace gets the seeded items but no UI prompt that names them. Comments now explicitly call out the in-flight state so readers between this PR and TASK-1150 know what's wired and what isn't. No behavior change. |
||
|
xarmian
|
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
|
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
|
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
|
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. |
||
|
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
|
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
|
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
|
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. |
||
|
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
|
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. |
||
|
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.
|
||
|
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.
|
||
|
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.
|
||
|
xarmian
|
1e94fcbd9d |
feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973) (#357)
* feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973)
Replaces raw stderr / status-text passthrough with a closed-set
ErrorCode taxonomy + structured ErrorEnvelope. Agents can now branch
on `error.code` instead of parsing free-form text:
{
"error": {
"code": "no_workspace",
"message": "No workspace context. Pass `workspace` explicitly, ...",
"hint": "Available workspaces: docapp, pad-web",
"available_workspaces": [{"slug": "docapp", "default": true}, ...]
}
}
Taxonomy (8 codes):
- no_workspace, unknown_workspace — populate available_workspaces
- auth_required, permission_denied
- item_not_found, validation_failed, conflict
- server_error (catch-all)
Implementation:
- internal/mcp/errors.go (new): ErrorCode constants, ErrorEnvelope +
ErrorPayload + WorkspaceHint types, NewErrorResult constructor,
classifyExecError + classifyHTTPStatus dispatchers, regex pattern
matchers for stderr classification, WorkspaceLister interface for
hint enrichment.
- internal/mcp/dispatch.go: ExecDispatcher.Dispatch routes failures
through classifyExecError (with itself as the WorkspaceLister).
Adds ListWorkspaces method that shells out to `pad workspace list
--format json`. Adds RootArgs field so the listing inherits root
flags (--url etc.).
- internal/mcp/dispatch_http.go: packageHTTPResponse routes 4xx/5xx
through classifyHTTPStatus. Lookup is intentionally nil here —
TASK-977 (PLAN-943) owns the privacy-preserving available_workspaces
filtering by OAuth allow-list.
- cmd/pad/mcp.go: pre-flatten rootFlags into RootArgs at
dispatcher construction.
NewErrorResult emits BOTH structured content (for Claude Desktop,
Cursor) AND a JSON text body (for older clients). Both decode to the
same envelope so wire-level shape stays uniform.
Tests:
- TestNewErrorResult_Envelope: round-trip the envelope through
structured + text surfaces.
- TestClassifyExecError: 11 cases covering every taxonomy code via
stderr patterns.
- TestClassifyExecError_LookupFailureStillReturnsEnvelope: lookup
failures degrade to empty available_workspaces, never drop the
whole envelope.
- TestClassifyHTTPStatus: 10 cases covering each HTTP status mapping
including the workspace-vs-item 404 fork.
- TestParseWorkspaceListJSON: happy path, empty / null / malformed,
entry-without-slug skipping.
- TestExtractUnknownWorkspaceSlug: regex helper round-trip.
Out of scope (per task description):
- HTTPHandlerDispatcher available_workspaces filtering by OAuth
allow-list → TASK-977 (PLAN-943).
- item_not_found "recent items" hint enrichment → also TASK-977.
- Per-code docs page on getpad.dev/mcp/local → TASK-976.
Parent: TASK-973 → PLAN-969.
* fix(cli): add JSON output to pad workspace list per Codex review (round 1)
Codex P2: classifyExecError's WorkspaceLister side channel calls
`pad workspace list --format json` to populate available_workspaces
in no_workspace / unknown_workspace error envelopes (TASK-973). The
CLI command silently ignored formatFlag and always printed the
human-readable shape, so parseWorkspaceListJSON would fail and the
hint was effectively never populated.
Add JSON branch to workspacesCmd that emits a {slug, name,
updated_at, default} array. The `default: true` flag marks the
CWD-linked workspace so agents can prefer it without a separate
DetectWorkspace call.
Manual verification:
$ pad workspace list --format json | jq '.[0]'
{
"slug": "docapp",
"name": "pad",
"updated_at": "2026-04-14T13:24:51Z",
"default": true
}
Parent: TASK-973 → PLAN-969.
* fix(mcp): tighten unknown-workspace slug regex per Codex review (round 2)
Codex finding: extractUnknownWorkspaceSlug's bare-word regex captured
stop-words like "not" out of generic "Workspace not found" messages.
The server emits exactly this generic body in middleware_auth.go and
handlers_workspaces.go, so the resulting envelope would say
`Workspace "not" is not visible to this session.` — pushing agents
toward retrying with a bogus slug.
Tighten the regex to only match QUOTED slug forms ("workspace 'foo'"
or "workspace \"bar\""). Bare-word phrasings yield empty slug, and
unknownWorkspaceResult now emits a generic "Workspace not visible to
this session." instead of the misleading empty-string `Workspace ""`.
Test cases updated:
- "workspace 'foo' does not exist" → "foo" (still works)
- "workspace \"bar\" not found" → "bar" (still works)
- "unknown workspace baz" → "" (was "baz", now intentionally empty)
- "workspace docapp not visible" → "" (was "docapp", now empty)
- "Workspace not found" → "" (the actual server response)
The other taxonomy / hint behavior is unchanged: ErrUnknownWorkspace
still classifies correctly, available_workspaces still populates from
ListWorkspaces, and the body text still appears in Hint via the
classifyHTTPStatus 404 branch's body-append logic.
Parent: TASK-973 → PLAN-969.
|
||
|
xarmian
|
9068e3e7da |
feat(mcp): document + lock workspace explicit-param precedence (TASK-972) (#356)
Workspace as an explicit per-call parameter was already wired in v0.2 —
every workspace-scoped tool's schema includes a `workspace` field, and
BuildCLIArgs implements (explicit > session > omit-and-let-CLI-find-CWD)
precedence. This commit closes the documentation + test gaps.
Changes:
- catalog.go: tighten the workspace param description to spell out the
three-step resolution order (explicit param > pad_set_workspace
session > CWD .pad.toml). Agents reading tools/list see why and how
to switch workspaces mid-session without a `cd`.
- catalog_workspace_precedence_test.go (new): three regression tests:
1. TestCatalogWorkspacePrecedence — drives pad_item.list through the
fan-out handler covering 3 of TASK-972's 4 cases (explicit-wins,
session-fallback, neither). Case 4 (no_workspace structured
error) is documented as out-of-scope here; it depends on
TASK-973's error taxonomy.
2. TestCatalogWorkspaceParamAdvertisedOnAllWorkspaceTools — every
ToolDef with Schema.Workspace=true must include `workspace` in
its tools/list schema. pad_meta is enumerated as the one
intentionally server-wide tool; future additions that drop
workspace by accident fail this test loudly.
3. TestCatalogWorkspaceDescriptionDocumentsPrecedence — pins the
schema description text so a future shortening doesn't drop the
substantive resolution-order detail.
Out of scope (per task description):
- no_workspace structured error envelope → TASK-973.
- Multi-workspace dogfood with Claude Desktop → manual verification.
Parent: TASK-972 → PLAN-969.
|
||
|
xarmian
|
eb896e4469 |
feat(mcp): server-level instructions advertised in initialize handshake (TASK-971) (#355)
Adds a top-level `instructions` string to the MCP initialize response so agents know WHEN to reach for pad without having to guess from tool descriptions alone. The Svelte MCP server in the dogfooding session that triggered PLAN-969 does this; pad now does too. Implementation: - internal/mcp/instructions.md (new) — embedded source content. MCP-aware adaptation of skills/pad/SKILL.md's opener: what pad is, when to reach for it, the v0.2 tool catalog summary, resource cheatsheet, workspace resolution order, ref convention, update flow, conventions hint, and the four prompts. - internal/mcp/instructions.go (new) — //go:embed wrapper exposing the content as the Instructions package var. - internal/mcp/server.go — pass server.WithInstructions(Instructions) into NewMCPServer. Single source of truth: the same string ships in both the local stdio handshake AND PLAN-943's HTTPHandlerDispatcher (when remote /mcp mounts in TASK-950 it will reuse the same constant — no docs drift between local and remote surfaces). Test: TestServer_InitializeAdvertisesInstructions drives a real initialize round-trip and asserts the response's Instructions field equals the embedded source. Sanity-checks the embed didn't truncate. Parent: TASK-971 → PLAN-969. |
||
|
xarmian
|
19f20c5911 |
feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981) (#354)
* feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981)
Final commit of TASK-970's 3-stage rollout (PLAN-969). pad_item lands
with 17 actions consolidating the v0.1 verb tools (item_create /
item_block / item_star / item_unstar / item_supersedes / item_unsupersede /
...) into one resource × action shape. cmdhelp leaf walker retired —
tools/list now advertises only the v0.2 catalog (~7 catalog tools +
pad_set_workspace).
pad_item actions:
- Lifecycle: create, update, delete, get, list, move
- Relationships: link, unlink, deps
- Stars: star, unstar, starred
- Comments: comment, list-comments
- Bulk + notes + decisions: bulk-update, note, decide
link / unlink dispatch on link_type via itemLinkRoutes table:
- blocks, blocked-by → item block / blocked-by + item unblock
- supersedes → item supersedes / unsupersede
- implements → item implements / unimplements
- split-from → item split-from / unsplit
Per-direction op (cmdPath, firstArg, secondArg, inverted) handles
the asymmetric "blocked-by unlink reuses unblock with operands swapped"
case correctly.
Walker retirement:
- registry.go shrinks dramatically. Register() now registers
pad_set_workspace + delegates to RegisterCatalog. Drop identifyLeaves,
hasExcludedAncestor, buildTool, makeDispatchHandler, propertyForArg,
propertyForFlag, propertyOptionsCommon, stringifyEnum, ToolNameFromPath,
DefaultExcludes, RegistryOptions.ExcludeCommands.
- mergeDispatchInput moves to dispatch.go (still used by env.Dispatch).
- registry_test.go pruned to: validation tests, MCPPropertyName tests,
shared helpers (fakeDispatcher, fixtureDoc, equalSlice). DOC-978
said to "delete and rebuild" — done; the v0.1 walker assertions
weren't worth carrying forward.
- cmd/pad/mcp.go: single Register() call (no separate RegisterCatalog).
ToolSurfaceVersion bumped 0.1 → 0.2. pad_meta.tool-surface's
rollout_status flips from "in-progress" to "complete" automatically
because the bump makes ToolSurfaceVersion != "0.1".
CLAUDE.md updated to reflect the new architecture (catalog over walker;
two version constants — CmdhelpVersion + ToolSurfaceVersion).
Tests:
- TestPadItemLink_DispatchTable iterates itemLinkRoutes and asserts
link/unlink dispatch correctly for every link_type, including the
blocked-by-uses-unblock-with-swapped-operands case.
- TestPadItemLink_Missing/UnknownLinkType for the structured error path.
- catalog_readonly_test.go's expected{} extended with pad_item
passThrough actions; link/unlink intentionally skipped (custom
dispatch).
- TestRegister_PassesPadVersionToCatalog round-trips PadVersion through
RegistryOptions → CatalogOptions → ActionEnv.
Parent: TASK-981 → TASK-970 → PLAN-969.
* fix(mcp): support repeatable refs for pad_item.bulk-update per Codex review (round 1)
Codex P (no priority shown — substantive issue): pad_item exposed
`ref: string` everywhere, but bulk-update's CLI takes a repeatable
positional (one or more refs). The retired cmdhelp walker generated
array schemas for repeatable args; v0.2's scalar `ref` made
bulk-update effectively single-item or schema-invalid for its
primary use case.
Fix: dedicated `refs: array<string>` schema param + custom
actionItemBulkUpdate handler. Translates `refs` array → repeatable
`ref` positional (the form BuildCLIArgs feeds CLI commands with
arg.Repeatable=true).
Why a separate `refs` param vs. overloading `ref`: keeps the schema
consistent across actions — agents see one shape per param name.
JSON Schema oneOf would also work but mcp-go's helpers don't expose
it cleanly.
Lenient fallback: a single ref passed unwrapped as a string still
works (logically equivalent to a 1-element array). Empty arrays and
missing refs both surface structured errors with `refs is required`.
Tests cover: array of strings → multiple positionals, single string
fallback, missing refs error, empty array error. Existing fixture
in TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath extended with
`refs: ["TASK-1", "TASK-2"]` so bulk-update reaches dispatch.
Parent: TASK-981 → TASK-970 → PLAN-969.
|
||
|
xarmian
|
fe05c170ea |
feat(mcp): v0.2 catalog read-only tools (workspace, collection, project, role, search) (TASK-980) (#353)
* feat(mcp): v0.2 catalog read-only tools (workspace, collection, project, role, search) (TASK-980)
Second commit of TASK-970's 3-stage rollout (PLAN-969). Adds 5 read-only
tools to the v0.2 catalog. v0.1 cmdhelp walker stays live alongside;
TASK-981 retires it.
Tools added:
- pad_workspace: list, members, invite, storage, audit-log
- pad_collection: list, create
- pad_project: dashboard, next, standup, changelog
- pad_role: list, create, delete
- pad_search: query (dispatches to "item search"; cross-workspace FTS)
Catalog adjustments from DOC-978's original list:
- Dropped pad_workspace.action: get — no equivalent CLI command exists.
- Dropped pad_collection.action: schema — schema is in `collection list`
output; can be added later if dogfooding shows demand.
Latent bug fix in fan-out handler:
- makeFanOutHandler now strips the catalog's `action` routing key from
the input map before invoking the action handler. Required because
some CLI commands (workspace audit-log) declare their own `--action`
flag — without stripping, BuildCLIArgs would silently emit
`--action audit-log` instead of the user's filter value. The bug was
latent in TASK-979 (pad_meta's actions don't dispatch) and surfaces
in TASK-980 with workspace audit-log.
Custom handler for workspace.audit-log:
- The CLI's `--action <filter>` flag would still collide if exposed
directly. Schema exposes it as `action_filter`; actionWorkspaceAuditLog
renames to `action` before dispatch. All other audit-log flags flow
through unchanged.
Tests:
- TestReadOnlyCatalog_AllToolsRegistered locks the new catalog entries.
- TestReadOnlyCatalog_ActionsMatchCmdhelp verifies every passThrough
cmdPath resolves in cmdhelp (catches drift at test time).
- TestPadWorkspaceAuditLog_RenamesActionFilter / _ForwardsWithoutFilter
pin the audit-log rename behavior in both directions.
- TestMakeFanOutHandler_StripsActionFromInput pins the strip behavior
so no future passThrough can leak the routing key.
Parent: TASK-980 → TASK-970 → PLAN-969.
* fix(mcp): tighten v0.2 catalog tests per Codex review (round 1)
Two P3 findings on test gaps in TASK-980's catalog_readonly_test.go:
1. TestReadOnlyCatalog_ActionsMatchCmdhelp checked a hardcoded `expected`
table against a hardcoded `liveCmdhelpDoc`. Catalog action drift
(rename, removal, addition) and CLI command renames could pass
silently because both halves were under test control. Now the test
does three-way validation:
- Every expected cmdPath resolves in liveCmdhelpDoc (catches typos
in our own table — the original check).
- Every catalog action (modulo inline-handling tools like pad_meta)
has an expected entry (catches new actions added without coverage).
- Every expected entry has a real catalog action (catches stale test
entries that outlive the action).
2. TestReadOnlyCatalog_AllToolsRegistered only verified expected names
were present; an accidental 7th tool would pass silently. Now fails
on unexpected entries AND on Catalog length mismatch.
A `skipTools` set lets us exclude pad_meta (whose actions are inline,
not dispatched) and document the exclusion. TASK-981 will extend the
expected{} map for pad_item.
Parent: TASK-980 → TASK-970 → PLAN-969.
* fix(mcp): exercise catalog actions through fake dispatcher per Codex review (round 2)
Codex P2: TestReadOnlyCatalog_ActionsMatchCmdhelp + AllToolsRegistered
were tightened in round 1, but they still didn't catch a class of drift —
e.g. flipping pad_search.query from passThrough([]string{"item","search"})
to passThrough([]string{"some","other"}) would pass the bijection check
because the action name still matches. The expected{} table was never
exercised against the actual handler.
Add TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath: invokes every
catalog action through a fake dispatcher with a maximal input fixture
(satisfies all required positionals across the read-only surface) and
asserts the captured cmdPath matches the expected table.
Closes the catalog → dispatch drift hole. Now if anyone changes a
passThrough cmdPath without updating expected{}, this test fails
loudly with the actual dispatched path printed in the error.
Parent: TASK-980 → TASK-970 → PLAN-969.
|
||
|
xarmian
|
df8a3631e7 |
feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) (#352)
* feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) First commit of TASK-970's 3-stage rollout (PLAN-969). Introduces the hand-curated v0.2 catalog types (ToolDef, ActionFn, ActionEnv) and ships one tool — pad_meta — end-to-end. v0.1 cmdhelp-walk surface stays live alongside; subsequent commits (TASK-980, TASK-981) migrate the rest and flip v0.1 off. Architecture record: DOC-978. The fan-out registry sits ABOVE the dispatcher boundary — Dispatcher / route table are unchanged, so both ExecDispatcher (stdio) and HTTPHandlerDispatcher (HTTP) inherit the new shape for free. Changes: - internal/mcp/catalog.go (new) — ToolDef, ActionFn, ActionEnv, passThrough helper, RegisterCatalog, makeFanOutHandler, structured error helpers. - internal/mcp/catalog_meta.go (new) — pad_meta tool with three inline actions: server-info, version, tool-surface (full catalog dump for PLAN-943 docs generation). - internal/mcp/version.go — add ToolSurfaceVersion = "0.2" + matching experimentalToolSurfaceKey. Independent of CmdhelpVersion (cmdhelp owns CLI help-tree contract; ToolSurfaceVersion owns MCP catalog). - internal/mcp/meta.go — extend MetaPayload with ToolSurfaceVersion; experimentalCapabilities advertises both padCmdhelp + padToolSurface. - cmd/pad/mcp.go — call RegisterCatalog alongside Register so v0.2 surface is live. - Tests: catalog_test.go + catalog_meta_test.go (new); meta_test.go + server_test.go updated to assert the new field/capability. Parent: TASK-970 → PLAN-969. * fix(mcp): keep ToolSurfaceVersion at "0.1" until catalog is complete per Codex review (round 1) Codex P1: advertising tool_surface_version=0.2 while the user-visible surface is still predominantly v0.1 (cmdhelp walker active alongside, only pad_meta in the catalog) misleads consumers that pin against the handshake or pad://_meta/version. The padToolSurface namespace would suggest the full resource/action shape is available when in reality only pad_meta uses it. Delay the 0.1 → 0.2 bump to TASK-981 — the commit that retires the cmdhelp walker and ships the complete catalog. The constant stays declared so the surface contract is wired through the handshake + meta resource + pad_meta.tool-surface, the version string just truthfully reflects "still v0.1" until the catalog is complete. No test changes needed: every assertion uses the constant, not a literal "0.2". Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): scope pad_meta.tool-surface to v0.2 catalog only per Codex review (round 2) Codex P1: pad_meta.tool-surface description claimed "Full catalog dump: every tool" but during PLAN-969's parallel rollout, tools/list contains both the catalog (currently just pad_meta) AND the cmdhelp walker's ~85 verb tools. Calling the catalog dump "every tool" misleads consumers who expect a complete enumeration. Same spirit as round 1's fix: stop claiming what isn't true. The catalog dump is the v0.2 catalog by design — consumers wanting the complete advertised surface should read tools/list directly. Hand-mapping the walker output into the catalog dump would cost duplication for a surface that's about to disappear in TASK-981. Wire-level changes: - Tighten the action description in padMetaToolDescription to say "v0.2 catalog dump: every tool managed by the hand-curated catalog" and explicitly note tools/list is the source for the complete surface. - Add rollout_status field to the response payload: "in-progress" while ToolSurfaceVersion stays at "0.1", "complete" once TASK-981 bumps it. Lets consumers detect the rollout state programmatically. - Test asserts the new field tracks ToolSurfaceVersion. Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): include params in pad_meta.tool-surface dump per Codex review (round 3) Codex P1: tool description claimed the dump includes each tool's "input schema" but the payload only emitted name/description/workspace/ actions[]. Misleading for docs generators (TASK-957) that would build getpad.dev/docs/mcp from this canonical source. Going with the substantive fix rather than just trimming the description: include a synthesized params[] per tool entry. Mirrors what consumers see in tools/list — `action` (always required, enum of declared action names), `workspace` (when ToolDef.Schema.Workspace=true), and per-tool ParamDefs. Synthesizing `action` and `workspace` rather than copying them from ToolDef makes the dump self-contained: a docs generator doesn't need to reproduce buildToolFromDef's implicit-param logic separately. Test asserts each catalog entry has params[] starting with `action` (enum length matches action handler count) and the right total length based on Schema.Workspace + Schema.Params. Parent: TASK-979 → TASK-970 → PLAN-969. |
||
|
xarmian
|
c70186547a |
feat(mcp): attachment list + show; reject upload/download/view as CLI-only (TASK-968 final slice) (#350)
* feat(mcp): wire attachment list + show; reject upload/download/view as CLI-only (TASK-968 partial)
Final slice of TASK-968's surface expansion. Two attachment commands
wired (the metadata-only ones), three rejected as noRemoteEquivalent
(those that take a local filesystem `<path>` argument), and the
noRemoteEquivalent map gets a small refactor to carry per-entry
rationale clauses.
New commands:
- attachment list → custom dispatcher with --item ref→UUID
resolution, --attached/--unattached
mutex fold, full filter pass-through
(category, collection, sort, limit,
offset).
- attachment show → HEAD /api/v1/workspaces/{ws}/attachments/{id}
packaging response headers
(Content-Type, -Length, -Disposition,
ETag, Last-Modified) into the CLI's
--format json shape:
{id, mime, size, filename?, etag?,
last_modified?}
Rejected as noRemoteEquivalent (with per-command rationale):
- attachment upload → needs a local filesystem `<path>` arg;
agents fetch raw bytes via the
attachment URL directly.
- attachment download → writes to a local filesystem `<out-path>`;
agents read raw bytes via the URL.
- attachment view → writes to a local filesystem path and
prints it; agents read bytes via the URL.
Refactor: noRemoteEquivalent went from `map[string]struct{}` to
`map[string]string` where the value is a rationale clause appended
to the error message. The previous generic message ("operates on
local pad client / config state, not the workspace") was misleading
for attachments (which DO operate on workspace state, just via a
local filesystem argument). Per-entry rationale lets each rejection
point at the alternative path agents should use — e.g. github
commands now suggest `item update --field github_pr=...`,
attachment commands point at `attachment show` + the URL.
The `--item TASK-5` resolution on `attachment list` reuses the
existing resolveItemRef helper (introduced in PR #346 for the link
commands), so the OAuth-scope hook (d.Apply) applies uniformly to
the prefetch — no scope bypass.
`parseAttachmentFilename` reproduces the CLI helper of the same
name. Handles both the bare `filename="value"` form and the RFC
5987 `filename*=UTF-8''<urlencoded>` form, preferring the latter
when both appear (spec-compliant carrier for non-ASCII names).
Tests:
- attachment list happy path with full query string forwarding
- --attached/--unattached fold + mutex rejection
- --item ref → UUID resolution end-to-end (and abort-on-resolution-
failure pin)
- attachment show: header → JSON extraction with all five fields
populated
- --variant query forwarding
- 404 surfaces as IsError
- parseAttachmentFilename: standard/quoted/unquoted/RFC5987 cases
- Per-entry rationale verified: github vs attachment messages
differ (the refactor's behavioural test).
- Integration smoke: attachment list against fresh workspace
(returns total=0); upload/download/view rejected with stable
"no remote equivalent" prefix.
TASK-968 is now functionally complete: 38 commands wired across
PR #346, #347, #348, #349, this PR, plus 13 commands explicitly
rejected as noRemoteEquivalent. The route table expansion lifts
the dispatcher from TASK-965's seed of 1 command to the full
mid-tier MCP surface PLAN-943's TASK-950 needs.
Parent: PLAN-943.
* fix(mcp): use mime.ParseMediaType for Content-Disposition parsing per Codex review (round 1)
Codex caught that the previous strings.Split(";") approach in
parseAttachmentFilename chopped quoted filenames containing semicolons
at the first internal `;`, returning `"a"` for
`attachment; filename="a;b.png"`. The CLI's helper of the same name
uses mime.ParseMediaType which respects the quote boundaries, so
the MCP path was diverging from CLI behaviour.
Fix: replace the hand-rolled splitter with mime.ParseMediaType +
filepath.Base(name) — matching the CLI exactly. mime.ParseMediaType
also handles the RFC 5987 `filename*=UTF-8''<urlencoded>` form
automatically, so we no longer need the explicit precedence check
either.
filepath.Base is the same defensive base the CLI applies even
though the server is supposed to sanitize before emitting the
header — keeps a stray `../` from sneaking through.
Tests added:
- PreservesSemicolonsInQuotedFilename pins the
`attachment; filename="a;b.png"` regression Codex flagged.
- AppliesBasenameDefense pins the directory-stripping behaviour
that filepath.Base provides.
Pre-existing tests (StandardForm, PrefersFilenameStarOverFilename,
HandlesQuotedAndUnquoted) still pass — mime.ParseMediaType handles
all three cases correctly.
Parent: PLAN-943.
|
||
|
xarmian
|
a5c58a5d66 |
feat(mcp): wire project standup + changelog + library activate (TASK-968 partial) (#349)
Continues TASK-968 past PR #348. Three more commands wired — the last of the project intelligence + library composition surfaces that needed multi-call composition. New commands: - project standup → multi-call: GET /dashboard + iterate terminal statuses via /items list + GET /items?status= in-progress. Filters completed by --days cutoff (default 1) client-side. Builds {date, days, completed, in_progress, blockers, suggested_next} matching the CLI's --format json output exactly. - project changelog → multi-call: iterates terminal statuses, filters by date (--since YYYY-MM-DD overrides --days, default 7) and parent (--parent ref/slug/title, case-insensitive across parent_link_id / parent_ref / parent_title). Groups by collection_slug, preserving first-seen order for stable output. - library activate → looks up convention/playbook by title via internal/collections.GetLibraryConvention / GetLibraryPlaybook (avoids two HTTP round-trips for the static library data), builds canonical fields blob via models.BuildConventionItemFields for conventions or {status,trigger,scope} for playbooks, POSTs into the workspace's conventions / playbooks collection. The standup + changelog dispatchers consolidate the multi-call patterns onto a small helper set: - listWorkspaceItems(ctx, user, workspace, query) for the per-status iteration. Best-effort error tolerance per call (matches CLI: a single failed status doesn't abort the standup). - itemUpdatedAfter for date-cutoff filtering. - itemMatchesParent for case-insensitive ref/slug/title parent filtering (mirrors the CLI's strings.EqualFold across three fields). - itemRefFromMap / itemTitleFromMap / extractItemFieldString / stringFromMap for the map[string]any decode path. The dispatcher operates on maps (not typed structs) for forward compat with server-side field additions, same approach as the slice 3 fix Codex caught in PR #348. Library activate routes through internal/collections package access rather than HTTPing the /convention-library + /playbook-library endpoints. Both paths return identical data (the handlers wrap the same constants), and the in-process accessor saves two round-trips per activate. The OAuth-scope hook still applies to the eventual POST so it's not a scope bypass. Tests: - Standup: shape pinned + cutoff filter (old item excluded) + --days default to 1. - Changelog: collection grouping + counts + --since overrides --days + parent filter + bad-since rejection. - Library activate: convention path (commits format, with seed metadata) + playbook fallthrough (Implementation Workflow) + not-found error + missing-input rejection. - Helpers: itemRefFromMap covers float64/int/int64/json.Number/ nil/string forms; extractItemFieldString handles malformed JSON; itemMatchesParent walks all three parent fields case-insensitively. - Integration smoke against real *server.Server: standup + changelog (empty workspace) + library activate (Conventional commit format from seed library). Cumulative TASK-968 progress: 36/~50 commands wired across PR #346, #347, #348, this PR. Remaining: attachments (5 commands, multipart bodies — separate PR). Parent: PLAN-943. |
||
|
xarmian
|
1fc41f2f4a |
feat(mcp): project intel + collection create + library list + bulk-update + note/decide (TASK-968 partial) (#348)
* feat(mcp): wire project intel + collection create + library list + bulk-update + note/decide (TASK-968 partial) Continues TASK-968 past PR #347's stars/roles/webhooks slice. Nine more commands land here, one more joins noRemoteEquivalent. New commands: - project next → alias /dashboard (matches CLI's verbatim --format json output) - project ready → custom: extracts suggested_next as {count, results} - project stale → custom: filters dashboard.attention to interesting types (stalled/blocked/ overdue/orphaned_task), sorted - collection create → custom: parses --fields DSL (key:type[:opts];...) into CollectionSchema, builds settings - library list → composes /convention-library + /playbook-library based on --type - item bulk-update → iterates refs with per-item RMW; per-item failures surface in results rather than aborting - item note → RMW append using models.AppendImplementationNote - item decide → RMW append using models.AppendDecisionLogEntry Extended noRemoteEquivalent: - project reconcile → shells out to `gh` CLI for live PR state, same locality reasoning as the github commands The project-intelligence dispatchers reproduce the CLI's --format json shapes exactly: - `next` returns dashJSON verbatim (CLI does the same) - `ready` returns {count, results} extracted from suggested_next - `stale` returns {count, results} after filterAgentAttention's type filter + (type, ItemRef, ItemTitle) sort Aliasing all three to /dashboard would diverge — agents would see an unexpected wrapper shape. `item bulk-update` mirrors the CLI's per-item RMW loop, including the "existing fields survive" guarantee. The response shape {updated, total, results[]} makes per-item outcomes available so the agent can inspect what succeeded vs. failed without re-querying. Per-item refs accept string / []string / []any for schema-permissive callers. `item note` / `item decide` reuse models.AppendImplementationNote / AppendDecisionLogEntry so CLI-created and MCP-created entries are indistinguishable. The created_by field uses the requesting user's name (or email fallback) so audit trails work in multi-user MCP deployments — the CLI hardcodes "user" since it's single-user-per- process. Tests: - Per-command happy + missing-input rejection. - project ready/stale shape pinned (count, results); stale's filter + sort order verified. - parseCollectionFieldsDSL pinned: title-cased labels, status:select auto-required+default, malformed entries rejected, empty input returns empty fields[]. - library list type-filter skips other endpoint when --type set; unknown --type rejected with clear error. - bulk-update: per-item failure doesn't abort batch; existing fields survive RMW; status/priority required gating. - note/decide: AppendImplementationNote/Decision entries land in fields with correct created_by user label. - project reconcile rejected with stable noRemoteEquivalent message. - Integration smoke against real *server.Server: create+bulk-update +note → project ready/stale → collection create → library list. Cumulative TASK-968 progress: 33/~50 commands wired across PR #346, #347, this PR. Remaining sections: project standup + changelog (multi-call composition); library activate (model-helper composition); attachments (multipart, separate PR). Parent: PLAN-943. * fix(mcp): preserve all dashboard.attention fields by switching to map-based decoding per Codex review (round 1) Codex caught that projectAttention's typed-struct round-trip dropped the `collection` field from the dashboard's attention entries — and would have dropped any future field additions silently. Same risk applied to projectSuggestion. Fix: stop decoding into a reduced typed struct. The dispatcher now unmarshals dashboard JSON into map[string]any, pulls named arrays via dashboardArrayField helper, and operates on the maps directly through filterAgentAttention's filter+sort. Result: every field the server emitted on each attention/suggestion entry survives to the response, no maintenance burden when handlers add new fields. filterAgentAttention now operates on []map[string]any with typed-string asserts at the comparator. Same filter set (stalled / blocked / overdue / orphaned_task) and same (type, item_ref, item_title) sort order — output ordering still pinned by the existing test. Test added: TestDispatch_ProjectStale_PreservesAllFields seeds an attention entry with every documented field PLUS a forward-compat `future_field` and asserts all of them flow through to the response. That regression-pins the wire-shape forwarder behaviour. Parent: PLAN-943. |
||
|
xarmian
|
e2127868d8 |
feat(mcp): stars + roles + webhooks + auth.whoami + workspace surface (TASK-968 partial) (#347)
* feat(mcp): wire stars + roles + webhooks + auth.whoami + workspace surface (TASK-968 partial) Continues TASK-968's route-table expansion past PR #346's link/lifecycle slice. Twelve more commands land here, plus the github commands move into the noRemoteEquivalent rejection set. New commands: - item star / unstar / starred (3 — stars) - role create / role delete (2 — admin role mgmt) - webhook list / create / delete / test (4 — webhook lifecycle) - auth whoami (1 — global, /auth/me) - workspace list / storage / audit-log (3 — workspace surfaces) - workspace invite (bonus — POST /members/invite) The simple-shape commands go through routeSpec; the few with custom shape (role create body, webhook create body, item starred's --all toggle, workspace audit-log's filter forwarding, workspace invite's default-respecting role omission) get their own RouteMappers. `auth whoami` and `workspace list` are global routes — the route-spec framework already supports paths without a {workspace} placeholder so they slot in cleanly. `workspace audit-log` hits /api/v1/audit-log (NOT scoped to workspace by URL — admin-only, can be filtered by ?workspace=<id> via input pass-through). `item star`/`unstar` rely on handleStarItem's store.ResolveItem accepting refs, slugs, AND UUIDs as the URL param — no prefetch needed. Extends noRemoteEquivalent with `github link`/`status`/`unlink`. These chain `git rev-parse` + the `gh` CLI to build the PR data they write — that data inherently lives in the agent's local checkout, so they can never have a useful remote equivalent. The error message points agents at the alternative path: use their own GitHub tools to fetch PR data, then `item update --field github_pr=...`. Tests: - Per-command happy path + missing-input rejection. - mapItemStarred default-vs-all behaviour for include_terminal. - mapRoleCreate and mapWorkspaceInvite omit empty optional fields so handler defaults aren't clobbered. - mapWorkspaceAuditLog forwards filters (action/actor/days/limit) and skips empty ones; verifies query string shape. - github commands rejected with stable noRemoteEquivalent message. - Integration smoke against real *server.Server: whoami, workspace list, create+star+starred, role create+delete, webhook list+create+delete. Parent: PLAN-943. * fix(mcp): drop session-default workspace forwarding for audit-log per Codex review (round 1) Codex caught a real divergence on PR #347: mergeDispatchInput auto- injects the session workspace into every MCP input map, so my mapWorkspaceAuditLog forwarding `input["workspace"]` as `?workspace=<id>` would silently scope audit-log calls to the session workspace by default. That's not what the CLI does — `pad workspace audit-log` deliberately omits the workspace filter so admins get the GLOBAL audit log. The fix matches CLI behaviour exactly: drop the `workspace` query-param forwarding. If a future CLI flag exposes workspace-scoped audit-log queries, it should surface as a separate input via cmdhelp (e.g. `--filter-workspace`) rather than folding-in the implicit session value. Test pin: TestMapWorkspaceAuditLog_DoesNotForwardSessionWorkspace asserts that workspace passed as input (the session-default form) does NOT show up as ?workspace= in the URL. The pre-existing pass-through test loses its workspace assertion since it's no longer forwarded. Parent: PLAN-943. * fix(mcp): normalize webhook events filter to JSON-array shape per Codex review (round 2) Codex caught that mapWebhookCreate forwarded the CLI's comma-separated --events string verbatim (e.g. "item.created,item.updated"), but the store persists events into a column that webhooks.matchesEvent unmarshals as a JSON array. Anything that isn't valid JSON-array syntax matches no events at all. The CLI is independently bugged here — its example `--events "item.created,item.updated"` produces a webhook that never fires — but the MCP path can produce a working webhook the first time. Add normalizeWebhookEvents: - Empty / missing → omit, let the store apply `["*"]` default. - Already a JSON array → pass through unchanged (no double-encoding). - Comma-separated → split, trim, encode as JSON array. - []any / []string from a schema-permissive caller → encode directly. - Malformed input → omit safely (caller falls back to default). Tests: - mapWebhookCreate normalizes comma-separated to JSON array; other fields untouched. - normalizeWebhookEvents passes through valid JSON, accepts array inputs, omits empty. - mapWebhookCreate omits events when missing (lets store default apply, matching the CLI's omit-flag behaviour). Parent: PLAN-943. |