268 Commits

Author	SHA1	Message	Date
xarmian	ba303e456f	fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381) ... Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically.	2026-05-02 18:36:55 -04:00
xarmian	69e471db8f	fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) (#380) ... * fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) Real MCP clients (Claude Desktop, Cursor as of 2026-05) don't send the RFC 8707 `resource` parameter on /oauth/authorize at all. Before this fix, translateResourceToAudience only translated resource→audience when resource= was present, so empty-resource requests reached fosite's audienceMatchingStrategy with an empty needle and got rejected with "resource parameter is required (RFC 8707)". fosite then redirected to the client's redirect_uri with ?error=invalid_request&error_description=..., and Claude's backend callback failed with the pydantic envelope "code: Field required" (because no `code` parameter was in the redirect query). RFC 8707 §2 marks the resource parameter OPTIONAL; servers with a single canonical audience are expected to default to it. pad's OAuth server has exactly one canonical audience by construction (cfg.MCPPublicURL + "/mcp"), so the right policy is to inject canonical when the client sends neither resource= nor audience=. Now translateResourceToAudience handles three cases in priority order: 1. audience= already set — leave both keys untouched. 2. resource= present — copy to audience= (existing path). 3. Neither present — inject canonical into both. The token gets bound to canonical exactly as if the client had sent it. audienceMatchingStrategy's strict empty-needle reject stays as defense in depth: case 3 only fires when canonical is configured (main.go won't construct the OAuth server otherwise), but if some future code path bypasses the translation helper, the matching strategy still fails loudly rather than minting an unbound token. Adds TestOAuth_Authorize_AcceptsNoResource_DefaultsToCanonical pinning Claude Desktop's exact request shape (no resource=, no audience=). Pairs with the existing AcceptsResourceOnly and audience-mismatch tests to lock in the full /authorize matrix. * docs(oauth): document RFC 8707 cross-server replay trade-off + audit log Per Codex review #383 round 1: defaulting to canonical when the client omits resource= weakens the cross-server replay defense RFC 8707 was designed to provide. Threat is the confused-deputy attack — malicious MCP server lies that pad's AS is its AS, client (which doesn't send resource=) drives a flow against pad's AS, pad mints a token bound to canonical, client returns it to the attacker, attacker replays at pad's /mcp. We're shipping with the default-to-canonical path because every real-world MCP client (Claude Desktop / Cursor / ChatGPT as of 2026-05) omits resource= and the alternative is "remote MCP doesn't work for any client until the entire ecosystem adopts RFC 8707." Mitigations now documented in the comment + active in the code: - Consent screen (TASK-952) is the trust anchor. Every grant requires a click-through that identifies the resource as "your Pad workspaces" and lists the user's actual workspace names. A user attempting to connect to a non-pad MCP server who lands on pad's consent screen sees the mismatch. - Matches industry practice (GitHub / Google / Atlassian all rely on consent-as-trust-anchor since RFC 8707 is barely deployed). - audienceMatchingStrategy's strict empty-needle reject stays as defense in depth — fires when canonical is unset and on any future code path that bypasses the helper. - Audit log (slog.Warn) on every default-fire gives ops a signal to detect anomalies — a spike of defaulted requests from a previously-unseen client_id is the earliest detectable shape of a confused-deputy attempt. Future task tracks restoring the strict reject once Claude / Cursor / ChatGPT all send resource=.	2026-05-02 16:50:34 -04:00
xarmian	9eb1a35f16	feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) (#379) ... * feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) Closes the last open work item in PLAN-943. HTTPHandlerDispatcher's unknown_workspace error envelope now populates available_workspaces filtered by the OAuth token's consent allow-list (TASK-952), so an agent never sees workspace slugs the user didn't explicitly grant. ## What changed - `HTTPHandlerDispatcher` gains a `Lister WorkspaceLister` field. Production wires `mcpserver.NewOAuthWorkspaceLister(s)`; tests can supply mocks. - `packageHTTPResponse` now takes a `lister` parameter and threads it down to `classifyHTTPStatus`. Both call sites in the package updated. - New `oauthWorkspaceLister` reads three things from request context: - `server.CurrentUserFromContext` — the requesting user. - `server.TokenAllowedWorkspacesFromContext` — the consent allow-list (TASK-953 plumbing). - `s.GetUserWorkspaces(user.ID)` — the user's full set. Returns the intersection. Wildcard (`["*"]`) and nil (PAT auth) short-circuit to "no filter" — the user's full set is returned in those cases since the token doesn't constrain workspaces. - `cmd/pad/main.go` wires the production lister. ## Privacy invariant A token whose allow-list is `[alpha, beta]` MUST NOT see "gamma" in the available_workspaces hint, even if the user is a member of gamma. Tested explicitly via TestUnknownWorkspace_AvailableWorkspaces_FilteredByAllowList — the test fakes a 4-workspace user membership, sets allow-list to 2, and asserts exactly 2 slugs appear in the filtered envelope. Without this filter, an attacker controlling an OAuth client could hit any random workspace slug, get the unknown_workspace envelope, and read OFF the user's full workspace list — defeating the whole point of the consent UI's per-workspace selection. ## Tests (18 new) 8 envelope round-trip tests pin every documented HTTP status → ErrorCode mapping (401 → auth_required, 403 → permission_denied, 404 generic → item_not_found, 404 workspace → unknown_workspace, 409 → conflict, 400/422 → validation_failed, 5xx → server_error, 418 → server_error fallback). 5 privacy-filter tests cover the allow-list shapes: specific-list-filters, wildcard-no-filter, no-allow-list-no-filter, no-user-empty-hints, store-error-empty-hints. 4 buildAllowSet unit tests for the helper. 1 end-to-end test through packageHTTPResponse. * fix(mcp): use req.Context() when packaging HTTP response (Codex round 1) Codex review #379 round 1 caught a real correctness issue: the packageHTTPResponse calls in executeRequest + the prefetch path in dispatchItemUpdate passed the dispatcher's outer ctx instead of req.Context(). The lister reads CurrentUser + TokenAllowedWorkspaces from context, and the canonical "everything attached" context is the SYNTHESIZED request's context — buildHTTPRequest layers WithCurrentUser + WithAPITokenAuth on it, and d.Apply (when wired) attaches token state on top of req specifically. In production this happened to work because MCPBearerAuth attaches TokenAllowedWorkspaces on the inbound /mcp request's context, which the dispatcher inherits as its outer ctx. But: - Tests driving executeRequest with context.Background() + a UserResolver-supplied user got empty available_workspaces because the outer ctx had no user. - Any future dispatcher attaching token state via Apply (rather than relying on inbound-ctx propagation) would also see the bug — the Apply hook is documented as the place for "TASK-953 token-scope context" exactly. Fix: pass req.Context() / prefetchReq.Context() to packageHTTPResponse. Same dispatcher, same ServeHTTP — just feed the lister the canonical post-Apply context. Test: TestExecuteRequest_UsesRequestContext_NotOuterContext drives executeRequest with an empty outer context + a UserResolver, asserts the resulting unknown_workspace envelope has the user's full workspace list. With the buggy version the test fails (lister sees no user → empty hints).	2026-05-02 15:35:18 -04:00
xarmian	3319ad5ea1	feat(mcp): per-token rate limit on /mcp (TASK-959) (#378) ... * feat(mcp): per-token rate limit on /mcp (TASK-959) Add a per-token rate limit to /mcp's auth middleware. Closes the "runaway agent burns through user quota" gap that PLAN-943 left as a follow-up to TASK-950. ## Policy - 60 requests / minute / token, burst 20. - Per-token (not per-IP): office-NAT-shared users don't share a bucket, and a runaway agent on one token can't burn another token's quota for the same user. - Limiter key: SHA-256(bearer) — the raw token never lives in the limiter map even though buckets persist for the 5-minute retention window. - Discovery docs (`/.well-known/oauth-*`) are NOT rate-limited. They're polled by MCP clients before any token exists; rate- limiting them per-IP would penalize office NATs and per-bearer doesn't apply (no bearer to hash). - No-bearer requests are 401'd before the limiter sees them, so a bare-bones DoS via empty Authorization headers gets the cheap rejection path without sharing a (necessarily-empty) bucket key. ## 429 response Per RFC 6585: `Retry-After: <seconds>` header (computed from the limiter's refill rate), plus `X-RateLimit-Limit`, `X-RateLimit-Remaining: 0`. Body is the MCP-shaped JSON envelope `{"error": {"code": "rate_limited", "message": "..."}}` so MCP clients (Claude Desktop, Cursor) can surface the error consistently. ## Implementation - `RateLimiters.MCPPerToken` — new `*ipRateLimiter` instance, drained in `Stop()` so cleanup goroutines don't leak (BUG-851 pattern). - `Server.checkMCPRateLimit` — called from `MCPBearerAuth` BEFORE auth validation. Returns false + writes 429 when bucket is exhausted; auth still 401s if the token is also invalid (the rate limit and validity checks are independent). - `hashTokenForLimiter` — SHA-256 hex digest helper. Uniform with the limiter's other (IP-string) keys. - `writeMCPRateLimit` — emits the 429 envelope. ## Tests - TestMCPRateLimit_PerToken_BucketEnforced — single token → 429 within 30 attempts (60/min, burst 20). - TestMCPRateLimit_PerToken_TwoTokensIndependent — drain token1 to 429, verify token2 still passes a full burst. - TestMCPRateLimit_DiscoveryDocsExempt — 50 hits to /.well-known/oauth-protected-resource, zero 429s. - TestMCPRateLimit_NoBearer_NotCounted — no-bearer requests 401 before the limiter, no 429s. - TestMCPRateLimit_429EnvelopeShape — Retry-After, X-RateLimit-* headers, MCP error envelope shape. - TestHashTokenForLimiter — hash determinism, length, no collision by prefix, empty input safety. * fix(mcp): move per-token rate limit AFTER auth validation (Codex round 1) Codex review #378 round 1 caught a memory-DoS risk: the pre-auth limiter created a new bucket entry for every distinct bearer string. An attacker rotating random bearer values would grow the limiter map unbounded until the 5-minute cleanup tick — millions of phantom entries before the goroutine catches up. Fix: relocate the checkMCPRateLimit call to AFTER auth validation in both PAT and OAuth paths. The limiter map now only fills with hashes of *valid* tokens, bounding map size by the active-token count rather than by the bearer-string space. Trade-off: invalid-bearer spam still hits the auth path's DB lookup (CPU cost, but a single indexed read per request) without any rate limiting. The CPU exposure is small enough to accept for v1; a follow-up could add a pre-auth per-IP cap for invalid-token flooding if real abuse appears. Tests: - TestMCPRateLimit_InvalidBearerNotRateLimited — 50 invalid bearers in a row, none get 429 (always 401). - TestMCPRateLimit_LimiterMapBoundedByValidTokensOnly — direct regression: 100 distinct invalid bearers, limiter map size must NOT grow. - Existing happy-path tests updated to use real PATs (via the new mustCreatePATForTest helper) so the post-auth-validation guard doesn't short-circuit them. * fix(mcp): move OAuth rate limit AFTER all validation gates (Codex round 2) Codex review #378 round 2 caught a P3 gap in round 1's fix. The OAuth path's rate-limit check ran AFTER IntrospectToken but BEFORE: - access-token-vs-refresh-token check - RFC 8707 audience match - session.GetSubject() presence - GetUser lookup So an active-but-not-authorized OAuth bearer (refresh token used as a bearer, wrong-audience token, deleted user) would create a limiter entry. After 30 such requests the response would flip from the intended 401 invalid_token to 429 — leaking limiter state to attackers and slightly defeating the bounded-map property. Fix: move the OAuth-path checkMCPRateLimit call to the very end of handleMCPOAuthAuth, just before context attachment + next.ServeHTTP. Now the limiter map only contains tokens that would have reached the dispatcher otherwise. Test: TestMCPRateLimit_OAuthRefreshTokenNotCounted — mints a real refresh token via the full OAuth flow, hammers /mcp with it 50 times, asserts every response is 401 AND the limiter map size is unchanged. * fix(mcp): move PAT rate limit AFTER all validation gates (Codex round 3) Codex review #378 round 3 caught the symmetric issue in the PAT path that round 2 fixed for OAuth. checkMCPRateLimit ran AFTER ValidateToken but BEFORE: - apiToken.UserID == "" check (legacy workspace-scoped tokens) - GetUser lookup (deleted-user case) Active-but-not-authorized PAT bearers (legacy tokens with no user_id, tokens whose user was deleted) would have created limiter entries and eventually 429'd instead of returning the intended 401 invalid_token. Fix: move the PAT-path checkMCPRateLimit call to the very end of handleMCPPATAuth, just before context attachment + next.ServeHTTP. Now mirrors the OAuth path's positioning — both run the rate limit exactly once, at the END of their happy path, so the limiter map only contains tokens that would otherwise reach the dispatcher.	2026-05-02 15:15:36 -04:00
xarmian	d01bbf6bf1	feat(oauth): live workspace allow-list + role enforcement (TASK-953) (#377) ... Closes the third leg of PLAN-943's OAuth permission model: (token capability tier) × (live workspace role) × (consent allow-list) The first two were already in place — TASK-1027 wired the tier scope check (pad:read / pad:write / pad:admin via tokenScopeAllows) and RequireWorkspaceAccess does the live role lookup. This PR adds the third gate: the workspace-allow-list set at consent time (TASK-952) actually denies workspaces NOT in the user's selection. ## What's new - `oauth.Session.AllowedWorkspaces()` / `SetAllowedWorkspaces()` — typed accessors on session.Extra. Handle BOTH the in-memory []string shape (consent-decide path) AND the JSON-decoded []interface{} shape (post-storage round-trip path). - `WithTokenAllowedWorkspaces` / `TokenAllowedWorkspacesFromContext` — context helpers in internal/server with defensive copies so callers can't corrupt the per-request token state. - `MCPBearerAuth` (OAuth path) reads the token's allow-list from session.Extra and stashes it in context. - `RequireWorkspaceAccess` checks the allow-list against the resolved workspace's slug. Three behaviours match TokenAllowedWorkspacesFromContext's return shapes: - nil → no token-level gate (PAT auth, pre-TASK-952 OAuth tokens). Standard membership applies. - ["*"] → wildcard. Every membership the user has passes. - [slug-a, slug-b, ...] → only listed slugs. Anything else gets 403 permission_denied BEFORE the membership check. ## Live role + revocation Membership revocation takes effect immediately. RequireWorkspaceAccess calls GetWorkspaceMember on every request — if the user lost membership in workspace X, the token's allow-list including X no longer helps; the request is rejected at the standard membership gate. Tested explicitly via TestWorkspaceAllowList_LiveMembershipRevocation. ## Tier × role The natural intersection of tokenScopeAllows (tier-based HTTP-method gate) and per-handler role checks (e.g. requireEditPermission) handles the tier × role table from the PLAN-943 spec: - pad:write tier passes tokenScopeAllows for POST. - But Viewer role fails requireEditPermission's role check. - Net: 403 — tested explicitly via TestWorkspaceAllowList_TierTimesRole_WriteByViewer. ## Tests Unit (no I/O): - TestTokenAllowedWorkspaceMatches — policy table for the helper. - TestWithTokenAllowedWorkspaces_DefensiveCopy + 1 reader counterpart. - TestSession_AllowedWorkspaces_*: setter/getter, nil-clear, defensive copy, JSON round-trip ([]string + []interface{} branches), wildcard JSON round-trip, not-set, nil-session. Integration (full chain, real OAuth flow): - TestWorkspaceAllowList_AllowsListedSlug — listed workspace passes. - TestWorkspaceAllowList_DeniesUnlistedSlug — unlisted gets 403 even though user is owner. - TestWorkspaceAllowList_WildcardAllowsAnyMembership — wildcard passes for every membership. - TestWorkspaceAllowList_LiveMembershipRevocation — token works, then membership revoked, then same token denied. - TestWorkspaceAllowList_PATPathUnaffected — PAT regression: PATs don't carry an allow-list, must NOT hit the gate. - TestWorkspaceAllowList_TierTimesRole_WriteByViewer — pad:write tier × Viewer role on POST item → 403.	2026-05-02 14:29:17 -04:00
xarmian	7d0de978f7	feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) (#376) ... * feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) Replace the inline-HTML stub from sub-PR C (TASK-1025) with the real consent page described in PLAN-943: server-rendered HTML with workspace multi-select, "any workspace" wildcard, and a capability- tier radio (read / write / admin). ## What the page does - Lists every workspace the user is a member of, with their role shown next to each row (informational — TASK-953 does live role resolution at MCP-call time). - Wildcard checkbox grants "any workspace I currently or later have access to," with a clear warning when checked. Mutually exclusive with per-workspace boxes (vanilla JS for UX, server-side rejection as the security gate). - Capability tier radio is constrained to the intersection of {pad:read, pad:write, pad:admin} and the client's requested scopes — fosite's grant-time subset check (RFC 6749 §3.3) rejects scopes outside the request, so the UI must never offer them. Default selects the highest tier the client requested. - Allow button stays disabled until ≥1 workspace (or wildcard) is selected. Server-side validation enforces the same rule regardless of JS state. ## Selective consent This is the central security property. The decide handler now grants *exactly* the chosen tier scope, NOT every requested scope. If the client requests `pad:read pad:write` and the user picks "read", the issued token has `scope=pad:read` only. Bonus fix: removed redundant scope re-grant loop in handleOAuthToken that would have expanded granted scopes back to the full requested set on every /token exchange — a real security bug that the auto-approve stub from sub-PR C masked because granted == requested for that flow. fosite's flow_authorize_code_token.go:134-138 + flow_refresh.go:91-103 copy GrantedScope/Audience automatically; our loop was undoing selective consent. ## Workspace allow-list storage The user's workspace selections live in `session.Extra["allowed_workspaces"]` (round-trips via storage.go's existing JSON marshal). Either `["*"]` for wildcard or a list of slugs. fosite's WriteIntrospectionResponse serializes Extra into the introspection response as top-level fields, so TASK-953's enforcement layer reads them off `/oauth/introspect` (or in-process via fosite.IntrospectToken). This sidesteps fosite's strict "granted ⊆ requested ⊆ client.Scopes" check — clients don't request `pad:workspaces:foo`, but the consent UI lets the user pick from their workspaces regardless. TASK-953 implements the live role resolution + workspace gate. ## Defense in depth - Server validates `capability_tier ∈ {read, write, admin}` AND that the chosen tier is among the client's requested scopes — fosite would reject otherwise with a less-readable error. - Server validates every non-wildcard slug is in the user's current membership table. A tampered form sending other slugs gets 400. - Wildcard wins: if a tampered POST sends both `*` and specific slugs, the result is `["*"]` only — never partial allow-list. ## Tests - TestConsent_RendersUserWorkspaces — multi-workspace list with role labels. - TestConsent_NoWorkspaces_ShowsEmptyState — clean empty state. - TestConsent_TierRadios_OnlyRequestedScopes — UI hides tiers the client didn't request. - TestConsent_ApproveWithSpecificWorkspaces — happy path, asserts introspection returns `allowed_workspaces=[alpha, beta]`. - TestConsent_ApproveWithWildcard — wildcard yields `["*"]`. - TestConsent_ApproveWithoutWorkspaceSelection_Rejected — 400 on empty allow-list. - TestConsent_ApproveWithUntrustedSlug_Rejected — defense in depth. - TestConsent_ApproveWithUnrequestedTier_Rejected — server tier validation matches UI's tier-radio constraint. - TestConsent_TokenScopeMatchesTierChoice_Read — selective consent: user picks read-only despite client requesting both, token has exactly `pad:read`. Existing tests + helpers updated to include the new consent fields (`capability_tier`, `allowed_workspaces`). * fix(oauth): prevent URL parameter pollution attack on consent UI (round 1) Codex review #376 round 1 caught a P1 security bug in the consent UI. The hidden-input round-trip used the full r.URL.Query() with only `csrf_token` stripped, so a malicious OAuth client could craft /oauth/authorize?...&capability_tier=admin&allowed_workspaces=* and the consent form would render those as hidden inputs BEFORE the user-controlled radios + checkboxes. On submit, the hidden values precede the user's selection in the form encoding, so: - r.FormValue("capability_tier") returns "admin" (first value matches the attacker's, not the user's) - r.PostForm["allowed_workspaces"] sees "*" first, the wildcard scan matches, the result is ["*"] regardless of which boxes the user actually checked Net effect: a user clicking through the consent UI for "read-only, just my docapp workspace" would silently authorize "admin, all workspaces" — without any visible cue that the values were wrong. Fix: build hidden inputs from an explicit allowlist of OAuth-standard authorize-request parameters (response_type, client_id, redirect_uri, scope, state, audience, resource, code_challenge, code_challenge_method, nonce). Anything outside the allowlist is silently dropped. This is strictly stronger than blocklisting consent-control names, because it also defends against future OAuth extensions adding new attacker- controllable params we haven't enumerated. Test: TestConsent_URLPollution_DoesNotOverrideUserSelection simulates the attack — GET /authorize with attacker params, asserts the rendered HTML contains zero `<input type="hidden" name="<attacker_name>">`, then completes the flow with the user's actual selection and confirms the issued token's scope matches the user's choice (pad:read), not the attacker's URL injection (pad:admin).	2026-05-02 14:08:11 -04:00
xarmian	924d82dae4	feat(oauth): MCPBearerAuth OAuth integration + public-info (TASK-1027) — closes TASK-951 (#375) ... * feat(oauth): MCPBearerAuth OAuth integration + public-info endpoint (TASK-1027, sub-PR E of TASK-951) Closes the OAuth server build-out by connecting sub-PRs A-D to the MCP transport from TASK-950 and shipping the consent-screen support endpoint. ## MCPBearerAuth OAuth path middleware_mcp_auth.go now branches on token shape: - pad_<60-hex> → existing PAT validation (TASK-950 path) - anything else → fosite.IntrospectToken via the new internal/oauth.Server.IntrospectToken wrapper (server-side, no HTTP roundtrip — pad-cloud is both auth server and resource server, so the public /oauth/introspect endpoint is for external clients only). OAuth path validation gates: - Token must be active (fosite returns ErrInactiveToken / ErrNotFound on revoked / unknown / expired tokens). - tokenUse must be access_token; refresh tokens explicitly rejected (RFC 6749 §1.5 — refresh tokens aren't bearers for resource calls). - Granted audience MUST contain the canonical MCP URL (RFC 8707 anti-replay; resource-server-side check defends against compromised or shared auth servers). - Subject must resolve to a real user row. Successful path stashes user + scopes via WithCurrentUser / WithTokenScopes. Scopes are translated from fosite's space-separated form to JSON-array form via oauthScopesToJSON. ## tokenScopeAllows pad:* extension Extended to recognize the OAuth scope vocabulary alongside PAT scopes: - pad:read ↔ read (GET/HEAD/OPTIONS only) - pad:write ↔ write (all methods) - pad:admin ↔ * (all methods) So MCP tool authorization stays uniform regardless of which transport issued the bearer. ## /api/v1/oauth/clients/{id}/public-info New read-only endpoint for the consent screen (TASK-952) and the OAuth-intent banner (TASK-1001, already shipped). Returns four non-sensitive fields: client_id, client_name, logo_uri, redirect_uris. - Auth-required (any logged-in user). - Cloud-mode-gated (404s outside cloud). - 404 for unknown clients. - Whitelisted leak surface — explicit fields, no embedded models.OAuthClient, so a future field addition (e.g. a confidential- client secret) doesn't accidentally appear here. ## Tests - TestMCP_OAuthAccessToken_Authenticates — happy path: full flow yields a token that authenticates against /mcp. - TestMCP_OAuthAccessToken_AudienceMismatch_Rejected — RFC 8707 resource-server check; mints a token, swaps the OAuth server for one with a different canonical, confirms 401. - TestMCP_OAuthRefreshToken_RejectedAtMCP — refresh tokens MUST NOT authenticate. - TestMCP_RevokedOAuthToken_Rejected — revocation takes effect at the resource server. - TestMCP_PATPath_StillWorks — regression for sub-PR D's coexistence with the OAuth path. - TestMCP_OAuthScopeReadOnly_StashesPadReadScope — scope round-trip. - TestOAuthClientPublicInfo_HappyPath / UnknownClient_404 / Unauthenticated_401 / NotMountedOutsideCloudMode — full coverage of the new endpoint. - TestE2E_ClaudeDesktopFlow — simulates the full sequence (discovery → DCR → authorize → token → /mcp call) Claude Desktop walks on first connect. - TestTokenScopeAllows extended with pad:* coverage. ## TASK-951 status Closes TASK-951 when this lands (5/5 sub-PRs done): - A: schema + storage layer (#370 / 2a00775) - B: fosite-backed authorization-server constructor (#371 / f6eeee4) - C: DCR + authorize + token endpoints + populated discovery (#372 / 48776a3) - D: revoke + introspect endpoints (#373 / 4250fb1) - E: MCPBearerAuth + public-info (this PR) * fix(oauth): fail-closed on empty OAuth scopes per Codex review (round 1) Codex caught a high-severity bug in oauthScopesToJSON: the helper mapped empty granted scopes to `[]`, which tokenScopeAllows interprets as the legacy "unrestricted" PAT shape (allow all methods). Combined with OAuth's RFC 6749 §3.3 rule that the `scope` parameter is OPTIONAL, this meant a client could: 1. Run the auth-code flow without requesting scopes. 2. Get back a token with empty granted_scopes. 3. Drive write MCP tools because MCPBearerAuth stashed `[]` and tokenScopeAllows fell through to the legacy unrestricted path. Fix: map empty OAuth scopes to JSON `null` instead. tokenScopeAllows denies on the "scopes == nil" branch (existing TASK-667 behavior), so the entire write surface is denied for empty-scope OAuth tokens. In production this path is hard to hit — sub-PR C's DCR handler defaults registered clients to `pad:read pad:write` when omitted, and audienceMatchingStrategy enforces canonical-audience matching at grant time. Defense-in-depth at the resource server is the right policy regardless. Test: TestOAuthScopesToJSON_FailClosedOnEmpty asserts both halves of the contract — the helper produces "null" for empty input, and tokenScopeAllows denies every method when fed that value.	2026-05-02 13:33:53 -04:00
xarmian	4250fb1976	feat(oauth): revoke + introspect endpoints (TASK-1026) (#373) ... * feat(oauth): revoke + introspect endpoints (TASK-1026, sub-PR D of TASK-951) Add the RFC 7009 revocation and RFC 7662 introspection endpoints, completing the spec'd surface that sub-PR C left as placeholders. - POST /oauth/revoke — fosite NewRevocationRequest delegates to our storage adapter's RevokeRefreshToken / RevokeAccessToken which walk the request_id (grant family) and mark every chain member inactive in one statement. Public clients authenticate by sending only client_id (token_endpoint_auth_method=none). - POST /oauth/introspect — fosite NewIntrospectionRequest with Bearer auth (a separate active access token). Returns {active:true, sub, scope, aud, client_id, exp, iat} for active tokens; bare {active:false} for unknown/revoked/expired (RFC 7662 §2.2 no-leak rule). Sub-PR E's MCPBearerAuth integration uses fosite.IntrospectToken directly server-side, but the public endpoint satisfies the discovery contract for clients that follow the chain. - Discovery doc populates revocation_endpoint + introspection_endpoint and their auth_methods_supported lists ("none" for both — public-clients-only model). Tests cover: - /revoke marks an access token inactive (verified via introspect). - /revoke on a refresh token revokes the entire grant family (paired access also goes inactive). - Refresh-token rotation: old pair becomes inactive, new pair active. - Refresh-token replay detection: replaying a rotated refresh kills the family (OAuth 2.1 §6.1, RFC 6819 §5.2.2.3). - Introspect happy path returns sub/scope/aud/client_id/exp. - Introspect on unknown token returns just {active:false} with no field leakage. - Introspect rejects requests with no Bearer Authorization header. - /revoke + /introspect 404 outside cloud mode. - Discovery doc advertises both endpoints + auth-methods lists. * fix(oauth): drop introspection_endpoint_auth_methods_supported per Codex review (round 1) Codex review of #373 caught a contradiction in the discovery doc: introspection_endpoint_auth_methods_supported: ["none"] advertised "no client authentication" for the introspection endpoint, but fosite's NewIntrospectionRequest rejects a request without Authorization: Bearer ... (and the test in this PR locks that in). A discovery-driven client would treat "none" as "post token+client_id unauthenticated" and get 401 — worse than no advertisement at all. Fix: omit introspection_endpoint_auth_methods_supported entirely. RFC 8414 §2 marks the field OPTIONAL; omission tells clients to negotiate auth out-of-band, which for our public-clients-only model means "send a separate active access token in the Authorization header." We document that in getpad.dev/mcp/local. revocation_endpoint_auth_methods_supported = ["none"] is kept and honest — fosite's NewRevocationRequest really does accept a public client posting only client_id (no Bearer required). * fix(oauth): RFC 7009 §2.2 idempotent revoke per Codex review (round 2) Codex caught that fosite v0.49 returns ErrInvalidRequest for the unknown-token path of NewRevocationRequest, which WriteRevocationResponse turns into 400. RFC 7009 §2.2 explicitly requires: "The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token." The 400 break the entirely normal "client retried after a previous revoke succeeded" or "operator typo'd the token" cases. Fix: detect the bare ErrInvalidRequest from the !found branch via isRevocationUnknownToken (which inspects HintField — fosite sets the hint on every other ErrInvalidRequest path it returns from NewRevocationRequest) and write 200 directly. Genuine malformed requests (wrong method, unparseable body, empty form) still return 400 because their ErrInvalidRequest carries a hint. Tests: - TestOAuth_Revoke_UnknownToken_Returns200 — locks in 200 for the unknown-token path. - TestOAuth_Revoke_MalformedRequest_Returns400 — counterpart that ensures the 200 override doesn't accidentally swallow real malformed-request errors. * fix(oauth): require token param + remove dead-code revoke override (round 3) Codex round 3 noticed that POST /oauth/revoke with client_id but no token returned 200 OK — silently swallowing a missing-required- parameter error. RFC 7009 §2.1 marks `token` REQUIRED. Investigating the fix surfaced that round 2's isRevocationUnknownToken override was actually dead code: fosite v0.49's handler/oauth2/revocation.go's RevokeToken collapses ErrNotFound + ErrInactiveToken to nil via storeErrorsToRevocationError, so NewRevocationRequest returns nil and WriteRevocationResponse writes 200 natively for unknown tokens. The override never fired in any real path. Cleanup: - Replace the unused isRevocationUnknownToken + override with a pre-check that returns 400 invalid_request when `token` is missing. RFC 7009 §2.1 enforced; fosite's native idempotency handles unknown tokens. - Update TestOAuth_Revoke_UnknownToken_Returns200's comment to reflect that it pins fosite's native behavior (not our override). - Add TestOAuth_Revoke_MissingToken_Returns400 to lock in the pre-check. - Keep TestOAuth_Revoke_MalformedRequest_Returns400 — verifies fosite's own ErrInvalidRequest paths still surface as 400.	2026-05-02 12:47:54 -04:00
xarmian	48776a3967	feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372) ... * feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951) Third of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. Mounts the three flow-driving HTTP endpoints over the fosite-backed server constructed in sub-PR B, replaces the TASK-950 501 stub with the real RFC 8414 discovery doc, and ships an inline-HTML consent stub as a TASK-952 placeholder so the auth-code flow runs end-to-end. What lands: - internal/server/handlers_oauth.go (744 LoC) - POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite. Public clients only (token_endpoint_auth_method=none rejected for any other value), authorization_code + refresh_token grants only, code response type only. Validates redirect_uris (absolute, no fragment, https or loopback-http or custom- scheme like claude://, blocks file:/javascript:/data:/vbscript:). - GET /oauth/authorize: starts auth-code flow. fosite validates request shape (PKCE-S256 required, audience matched, redirect exact-match). If user has session → renders inline consent stub. If not → 302 to /login?redirect=<self> (TASK-998's plumbing in pad-cloud honors the redirect=). - POST /oauth/authorize/decide: processes consent decision. Form-bound CSRF token (the existing __Host-pad_csrf cookie, read from a hidden form field instead of header). Approve → fosite NewAuthorizeResponse → 303 to client.redirect_uri with code. Deny → fosite WriteAuthorizeError(access_denied). - POST /oauth/token: code + refresh exchange. fosite verifies PKCE verifier (S256-required) + RFC 8707 audience. Returns {access_token, token_type, expires_in, refresh_token, scope}. RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS issues on authorize-code grant. - Inline consent stub: minimal HTML form with Approve/Deny, auto-grants every requested scope (TASK-952's UI replaces with workspace allow-list selection per TASK-953). - internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub → handleOAuthAuthorizationServer. Returns RFC 8414 metadata with all six endpoint URLs (revoke + introspect URLs sub-PR D fills with handlers; the URLs are stable now), advertised scopes, S256-only code_challenge_methods, resource_indicators_supported=true, authorization_response_iss_parameter_supported=true. - internal/server/server.go: Server.oauthServer field + SetOAuthServer + registerOAuthRoutes called from setupRouter inside an r.Group with requireCloudMode + SessionAuth (so /authorize can detect the logged-in user via __Host-pad_session; SessionAuth falls through gracefully when no cookie). - cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL being set (the OAuth surface needs a canonical audience to bind tokens to). CSRF posture: middleware_csrf.go runs only on /api/* paths so /oauth/* is naturally exempt. The consent decision endpoint adds its own form-token check (validateConsentCSRFToken) using the same __Host-pad_csrf cookie the SPA uses, just with the token in a hidden form field rather than a header. Same security model, different transport. Tests (12, all passing): - TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins RFC 8414 metadata fields including S256-only PKCE + resource_indicators_supported. - DCR (5): happy path; missing redirect_uris; bad redirect-URI shapes (relative, non-loopback http, fragment, javascript:); non-public client auth method rejected; unknown grant type rejected; not mounted outside cloud mode. - /authorize (3): redirects to /login when no session; renders consent stub when logged in; rejects audience mismatch via fosite's audienceMatchingStrategy. - /authorize/decide (2): rejects missing csrf_token; deny produces access_denied redirect. - Full PKCE flow: end-to-end /authorize/decide (approve) → /token with code_verifier → 200 with access+refresh tokens. - /token: rejects missing PKCE verifier. Replaces the 501 stub assertion in TestMCP_AuthServerStub with TestMCP_AuthServerMetadata_Mounted (just confirms 200; full shape lives in the OAuth-handler test). Out of scope: - /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026) - MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027) - Real consent UI with workspace allow-list (TASK-952) * fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1) Two findings from PR #372 round 1: 1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT) send `resource=` not `audience=`. fosite v0.49 reads only `audience` from the form, so audienceMatchingStrategy was hit with an empty needle and rejected every real-world authorize / token request. Tests masked the gap by sending both keys. Fix: translateResourceToAudience() copies r.Form["resource"] into r.Form["audience"] before each handler invokes fosite. Idempotent — if both keys are present, audience wins (test harness sends both for belt-and-suspenders). Applied at /authorize, /authorize/decide, and /token entry points. Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY resource= (no audience=) and asserts the request reaches the consent stub. Without the translation it 303s with invalid_request. 2. P2: /.well-known/oauth-authorization-server advertised /oauth/revoke + /oauth/introspect endpoints that don't exist yet (sub-PR D wires them). Real clients dialing those URLs would get 404. RFC 8414 §2 lists revocation_endpoint + introspection_endpoint as OPTIONAL, so omitting until the handlers ship is spec-compliant + honest. Fix: drop revocation_endpoint, introspection_endpoint, and their *_endpoint_auth_methods_supported counterparts from authServerMetadata. Sub-PR D's PR description includes "populate these here" as a follow-up. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints asserts the four fields are absent. * fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2) Two findings from PR #372 round 2: 1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop / Cursor self-register without prior auth) but had no rate limit. An attacker could flood the oauth_clients table indefinitely. Fix: extend RateLimit middleware to gate /oauth/register at the same 5/hour/IP rate the existing /api/v1/auth/register uses (RateLimiters.Register, burst 5). Added the OAuth route group to the s.RateLimit middleware chain so the new path actually runs through the limiter. Other /oauth/* endpoints aren't rate-limited here: /authorize rides session cookies (cheap to abuse but ineffective without a logged-in user), /token is PKCE-bound to a stored code (single-use), /authorize/decide is form-bound. Explicit per- endpoint /oauth/* limits arrive with TASK-959. Test TestOAuth_Register_RateLimited fires 5 requests successfully, asserts the 6th returns 429. 2. P2: Discovery doc advertised authorization_response_iss_parameter_supported=true, but the /authorize success path delegates to fosite v0.49 which doesn't add iss=<issuer> to the redirect. RFC 9207-aware clients seeing the flag would treat the missing parameter as a protocol violation. Fix: drop the field from authServerMetadata. RFC 8414 §2 marks it OPTIONAL — omission is spec-compliant. We'll add the parameter (+ post-processing of fosite's response) in a future PR if a real client requires it; today's MCP clients (Claude Desktop, Cursor, ChatGPT) don't. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints extended to cover the field. * fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3) Codex round 3 caught: /.well-known/oauth-authorization-server lives in the MCP route group (registerMCPRoutes), while the /oauth/{ register,authorize,token} handlers live in the OAuth route group (registerOAuthRoutes, gated on s.oauthServer != nil). A cloud deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted but NOT OAuth — the discovery doc would 200 with /oauth/* URLs that 404. Worse for clients than no document at all. Fix: handleOAuthAuthorizationServer now also nil-checks s.oauthServer; on nil it returns 503 with config_error, matching the existing fail-loud branch for when the issuer URL isn't configured. Ops detect the misconfiguration immediately rather than fielding "OAuth registration is failing with 404" tickets. Test: - TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled builds a Server with SetCloudMode + SetMCPTransport (so the MCP route group mounts) but NOT SetOAuthServer; asserts the endpoint returns 503 with config_error. - TestMCP_AuthServerMetadata_Mounted renamed → TestMCP_AuthServerMetadata_MountedAndGated to reflect the new behavior under mcpEnabledTestServer (which doesn't wire OAuth). The full 200 happy path lives in TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses oauthEnabledTestServer). * fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known * fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485 CI govulncheck rejected the build: fosite v0.49.0 transitively pulls github.com/go-jose/go-jose/v3@v3.0.3 which has GO-2025-3485 (DoS in JWS parsing). Affected call site: internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned. Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory). go mod tidy auto-bumped dependent indirect deps too. Verified locally: govulncheck ./... → "No vulnerabilities found" go test ./... → all green go build ./... → clean	2026-05-02 11:56:57 -04:00
xarmian	f6eeee4f81	feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951) (#371) ... * feat(oauth): fosite-backed authorization-server constructor (TASK-1024, sub-PR B of TASK-951) Second of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. Wires fosite v0.49.0 over the storage layer from sub-PR A. No HTTP routes yet — sub-PR C mounts /authorize, /token, /register; sub-PR D mounts /revoke and /introspect. What lands: - internal/oauth/session.go — pad's *Session embedding fosite.DefaultSession with typed UserID() accessor + Clone override returning *Session (so handler-side type-assertions don't lose the concrete type during refresh-token rotation). - internal/oauth/storage.go — Storage adapter satisfying: fosite.ClientManager handler/oauth2.AuthorizeCodeStorage handler/oauth2.AccessTokenStorage handler/oauth2.RefreshTokenStorage handler/oauth2.TokenRevocationStorage handler/pkce.PKCERequestStorage Compile-time guards in server.go assert each interface remains satisfied. Translation: fosite.Requester ⇄ models.OAuthRequest via JSON-encoded session_data + URL-encoded form. Sentinel errors from sub-PR A map to fosite.ErrNotFound / ErrInvalidatedAuthorizeCode / ErrInactiveToken. - internal/oauth/audience.go — RFC 8707 custom AudienceMatchingStrategy. fosite has no native RFC 8707; we close over a canonical audience and reject any request that doesn't carry exactly that resource. Belt-and-suspenders haystack check defends against fixtures / migrations that register a client without setting Audience. Plus ValidateAudienceParam (HTTP-handler entry helper) and audienceForNewClient (DCR seed for sub-PR C). - internal/oauth/server.go — NewServer(Config) → *Server returning fosite.OAuth2Provider configured for: - PKCE-S256 required (EnforcePKCE + EnablePKCEPlainChallengeMethod=false) - Opaque HMAC tokens (compose.NewOAuth2HMACStrategy) - Refresh rotation with grant-family revocation (sub-PR A round-2 fix) - Audience binding via the custom strategy Sensible default lifespans (1h access, 30d refresh, 15m authcode); overridable via Config. Excluded by design: client-credentials, implicit, ROPC (deprecated in OAuth 2.1), OpenID factories (we're not OIDC), PAR (not needed for v1). - go.mod — github.com/ory/fosite pinned at v0.49.0 (direct dep). Tests (20): - NewServer required-field validation (3) + default-lifespan path - audienceMatchingStrategy: empty needle, mismatch, client-without-canonical, canonical-only happy path, multi-audience rejection, no-canonical=ServerError - ValidateAudienceParam (5 sub-cases) + audienceForNewClient - Session: clone returns concrete *Session (not *DefaultSession); nil-safe accessors - Storage adapter: auth-code round-trip + invalidated-code error, GetClient not-found mapping, access-token-inactive mapping, rotation-revokes-entire-grant (end-to-end), PKCE round-trip, requester-to-OAuthRequest session encoding + missing-client guard Out of scope (subsequent sub-PRs): - HTTP route handlers + DCR endpoint (sub-PR C / TASK-1025) - /revoke + /introspect endpoints (sub-PR D / TASK-1026) - MCPBearerAuth OAuth introspection branch (sub-PR E / TASK-1027) * fix(oauth): inject canonical audience into hydrated clients per Codex review (round 1) Codex round 1 caught a P1 in the storage adapter: modelClientToFosite returned fosite.DefaultClient.Audience=nil for every persisted client, but audienceMatchingStrategy's haystack-side check requires client.GetAudience() to contain the canonical audience. Net result: every authorize / token / refresh flow would fail with invalid_request once the strategy ran, regardless of how the client was registered. Fix: thread the canonical audience through Storage. NewStorage now takes a canonicalAudience string; modelClientToFosite (now a method on Storage) injects [canonicalAudience] into the hydrated client's Audience field. The audience isn't persisted as a column — single-resource AS for v1 (PLAN-943) means every client implicitly allows the same audience, so storing what we'd always set to the same value is pure write amplification. Threading: cfg.AllowedAudience → NewServer → NewStorage(store, audience) └─ Storage.canonicalAudience └─ modelClientToFosite injects Misconfigured Storage (empty canonicalAudience — caught earlier by NewServer's required-field check, but tests pin the fail-loud branch in case Storage is ever constructed directly): produces clients with Audience=nil so audienceMatchingStrategy rejects every request with ServerError, surfacing the misconfiguration fast rather than silently issuing wide-open tokens. Tests: - TestStorage_GetClient_InjectsCanonicalAudience — pins the injection contract; without the fix this fails. - TestStorage_NewStorage_EmptyCanonicalLeavesAudienceNil — pins the fail-loud branch for misconfigured Storage. - 6 existing tests updated to pass canonical audience to NewStorage (mechanical sed update; behaviour unchanged). * fix(oauth): hydrate request payload on inactive token Get*Session per Codex review (round 2) Codex round 2 caught a HIGH-severity gap: GetRefreshTokenSession returned (nil, fosite.ErrInactiveToken) for revoked rows, but fosite's handleRefreshTokenReuse (flow_refresh.go:178-204) derefs req.GetID() to drive the family revocation that's the OAuth 2.1 BCP §4.14 replay-detection rule. Returning nil nil-derefs that flow and defeats replay detection — the very thing rotation exists to enable. Fix: hydrate the stored row even on the inactive path and return (req, fosite.ErrInactiveToken). Mirrors the pattern already used by GetAuthorizeCodeSession's invalidated-code branch. If hydration itself fails (client deleted between issuance and use), return the underlying error rather than masking it — replay detection loses but the failure is observable. Symmetric fix applied to GetAccessTokenSession even though no fosite caller currently derefs on inactive there. Defense in depth + uniform contract makes the adapter resilient to future fosite changes (e.g. an introspector that wants req.GetID() for audit-log enrichment). Tests: - TestStorage_GetRefreshTokenSession_InactiveReturnsPayload — pins the refresh-side contract; without the fix this fails on the nil-check. - TestStorage_GetAccessTokenSession_InactiveReturnsPayload — same pattern for access tokens. * fix(oauth): set RefreshTokenScopes=[] so authorize-code grants issue refresh per Codex review (round 3) Codex round 3 caught a P1: fosite defaults Config.RefreshTokenScopes to ["offline", "offline_access"]. fosite only mints refresh tokens when one of the listed scopes is granted. PLAN-943's scope vocabulary is pad:read / pad:write / pad:admin — no "offline" scope — so the default silently disabled refresh issuance for every Pad grant, defeating the entire refresh-rotation + family-revocation machinery this PR adds. Fix: explicitly set RefreshTokenScopes: []string{} in NewServer's fosite.Config. fosite reads the empty slice as "issue refresh on every authorize-code grant whose client allows the refresh_token grant type, no scope predicate" — matches fosite's own tests (flow_authorize_code_token_test.go:129). Pin: TestNewServer_RefreshTokenScopesIsEmpty documents the decision + smoke-checks that the constructor still returns a usable provider. The actual "refresh issued on authorize-code grant" assertion lands in sub-PR C's /token endpoint test — that's where fosite's flow_authorize_code_token.go reads the field. * fix(oauth): bump go.opentelemetry.io/otel{,/sdk} to v1.40.0 to resolve GO-2026-4394 CI govulncheck job rejected the build: fosite v0.49.0 transitively pulls in go.opentelemetry.io/otel/sdk@v1.21.0 which has known vulnerability GO-2026-4394 (Arbitrary Code Execution via PATH Hijacking in go.opentelemetry.io/otel/sdk). Affected init-time call sites: internal/oauth/audience.go:8 → fosite.init → otel resource.init internal/server/middleware_ratelimit.go:80 → sync.Once.Do → resource.Default internal/cli/client.go:415,794,798 → otelhttp.* → trace.* Fix: bump otel core + sdk + metric + trace to v1.40.0 (the fixed version per GO-2026-4394's advisory). go mod tidy also pulled in go.opentelemetry.io/auto/sdk@v1.2.1 as a new transitive. Verified locally: govulncheck ./... → "No vulnerabilities found" go test ./... → all green go build ./... → clean	2026-05-02 10:59:07 -04:00
xarmian	2a00775481	feat(oauth): schema + storage layer (TASK-1023, sub-PR A of TASK-951) (#370) ... * feat(oauth): schema + storage layer for OAuth 2.1 server (TASK-1023, sub-PR A of TASK-951) First of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. This one is foundation only — no HTTP exposure, no fosite import, no public surface change. Schema (5 tables, parallel SQLite + Postgres migrations): - oauth_clients — RFC 7591 Dynamic Client Registration; public clients only for v1 - oauth_authorization_codes — short-lived codes for the auth-code grant - oauth_access_tokens — opaque HMAC; subject denormalized for fast user-bound queries - oauth_refresh_tokens — same shape; access_token_signature link + request_id chain - oauth_pkce_requests — PKCE session keyed by auth-code signature Storage layer (internal/store/oauth.go): - 12 public methods covering fosite's ClientManager + CoreStorage + PKCERequestStorage + TokenRevocationStorage interface shapes, using pad-internal types so the package stays fosite-free. - Three sentinel errors (ErrOAuthNotFound, ErrOAuthInvalidatedCode, ErrOAuthInactiveToken) that sub-PR B's adapter maps to the matching fosite errors. - request_id IS the chain identifier (fosite preserves it across rotations — handler/oauth2/flow_refresh.go:86), so family revocation is a single indexed UPDATE rather than a separate chain_id column. 14 tests covering: client CRUD + idempotent delete + empty-slice normalization, auth-code create/get/invalidate (including the "return payload alongside ErrInvalidatedCode" contract fosite relies on for family revocation), access-token CRUD + delete, refresh CRUD + RotateRefreshToken (single-row flip), refresh-token family revocation (entire chain via request_id, leaves other chains untouched), access-token family revocation, PKCE CRUD, and required-field validation. Both backends share test bodies via testStore(t); set PAD_TEST_POSTGRES_URL=... to run the same suite against Postgres. Out of scope for this PR (subsequent sub-PRs): - fosite import + adapter (sub-PR B / TASK-1024) - DCR + authorize + token endpoints (sub-PR C / TASK-1025) - revoke + introspect endpoints (sub-PR D / TASK-1026) - MCPBearerAuth OAuth integration (sub-PR E / TASK-1027) * fix(oauth): always insert active=true; drop broken zero-value Active override per Codex review (round 1) Codex round 1 caught a P1 in insertOAuthRequestRow: active := defaultActive if req.Active != defaultActive { active = req.Active // <- zero-value collides } When defaultActive=true and req.Active=false (the zero value), this branch fires and the row is stored with active=FALSE — silently producing immediately-revoked tokens. Any sub-PR B adapter that built an OAuthRequest without explicitly setting Active=true would ship broken. Fix: hardcode active=TRUE on insert. Drop the defaultActive parameter (it's always true for the three flagged tables; PKCE has no active column). Pre-seeding inactive isn't a supported flow — fosite never does it, and tests that need a revoked row do Create + Invalidate / Rotate / RevokeFamily as a two-step. Regression test TestOAuth_Insert_AlwaysActive constructs an OAuthRequest with zero-value Active and asserts the row is readable as active for all three table types (codes, access, refresh). Without the fix the test fails on the first GetAccessToken call with ErrOAuthInactiveToken. * fix(oauth): RotateRefreshToken revokes both refresh + access families per Codex review (round 2) Codex round 2 caught: my RotateRefreshToken only marked the named refresh row inactive, but fosite's reference MemoryStore.RotateRefreshToken (storage/memory.go:497-504) revokes BOTH the refresh family AND the access family for the grant's request_id. Without this, every access token issued before a refresh remained active until TTL — defeating the rotation's invalidation contract. Fix: RotateRefreshToken now delegates to RevokeRefreshTokenFamily + RevokeAccessTokenFamily (both already existed). The signatureToRotate parameter becomes vestigial — fosite passes it but the family revoke catches every chain member regardless of which row triggered the rotation. The new pair fosite immediately issues via CreateAccessTokenSession + CreateRefreshTokenSession inherits the same request_id (flow_refresh.go:86) and lands active=TRUE per the round-1 hardcode, so the net post-rotation state is "all old rows in this grant inactive, the new pair active." Test rewrite: TestOAuth_RotateRefreshToken_FlipsActiveOnSingleRow asserted the OPPOSITE behavior (only one row touched) — that was the original bug. Replaced with TestOAuth_RotateRefreshToken_RevokesEntireGrant which seeds a refresh + access pair in the same chain, plus a distinct unrelated grant, then asserts after rotation: - old refresh + old access both inactive - unrelated grant untouched (request_id-scoped) * fix(oauth): DeleteOAuthClient cascades dependent rows in a tx per Codex review (round 3) Round 3 finding: DeleteOAuthClient errored with FK constraint violation for any client that had ever issued a grant. The migrations declare client_id FKs without ON DELETE CASCADE — by design, so a stray DELETE FROM oauth_clients elsewhere fails loudly rather than silently nuking grants — but that meant the "officially supported" delete path was unusable. Fix: DeleteOAuthClient now runs five sequential DELETEs inside a single transaction: 1. oauth_pkce_requests 2. oauth_refresh_tokens 3. oauth_access_tokens 4. oauth_authorization_codes 5. oauth_clients Order matters (children before parent) because the FKs aren't cascading. The tx makes it atomic — if any step fails, nothing's deleted, so we never leave a half-deleted client. Idempotent because every WHERE matches nothing on a non-existent client. Test TestOAuth_DeleteOAuthClient_CascadesDependentRows seeds a row in each of the four dependent tables, deletes the client, and asserts ErrOAuthNotFound on every dependent row + the client itself. Without the fix this fails on the first DELETE FROM oauth_clients with an FK constraint violation. * fix(oauth): SELECT FOR UPDATE row lock in DeleteOAuthClient on Postgres per Codex review (round 4) Codex round 4 caught a Postgres race in DeleteOAuthClient: the five-DELETE cascade is atomic, but between the child-row deletes and the parent delete, a concurrent fosite handler can insert a fresh grant/token referencing the same client_id. The parent DELETE then fails with an FK violation and the whole tx rolls back — the cascade is correct, but unreliable under concurrent OAuth issuance. Fix: take SELECT id FROM oauth_clients WHERE id = ? FOR UPDATE as the very first statement in the tx (Postgres only). The exclusive row-level lock blocks any concurrent statement that tries to read the client row — which fosite does on FK resolution during grant/token inserts — until our tx commits. Skipped on SQLite because: (a) BEGIN IMMEDIATE serializes the entire write workload globally (DSN configures _txlock=immediate per store.go), so the race doesn't exist. (b) FOR UPDATE syntax isn't reliably accepted across SQLite drivers. ErrNoRows on the lock query is treated as "client doesn't exist yet" — the subsequent DELETEs match nothing and the call remains idempotent. Tests still pass on the SQLite path; the Postgres path's race fix will be exercised by CI's PAD_TEST_POSTGRES_URL runs and any future concurrency test we add.	2026-05-02 10:25:53 -04:00
xarmian	521853e0a1	feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369) ... * feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied.	2026-05-02 09:25:25 -04:00
xarmian	48bbe7453b	fix(dashboard): suggested_next surfaces in-progress + filters blocked items (BUG-990) (#366) ... Pre-fix algorithm only considered status == "open" child items in active plans. Two consequences agents flagged in BUG-987 / BUG-990: 1. In-progress items never appeared. The most likely "what should I work on next" answer is "the work the user is already on" — pre-fix the engine returned [] when nothing was open AND a task was actively in-progress. 2. Blocked items appeared. Suggesting work that has unresolved blockers wastes the user's time when they go to start it. Algorithm changes (internal/server/handlers_dashboard.go): - Include both `open` and active-status (in-progress / fixing / exploring / etc., via existing isActiveStatus helper) child items. - Filter out items with at least one active "blocks" link from a non-done blocker. Mirrors the attention-section logic, factored into a new itemBlockedByActive helper. - Sort: in-progress first (always wins over open, regardless of priority), then by priority rank within each bucket. Pinned in test expectations. - Reason text distinguishes "In-progress task..." vs "Open task..." so agents see why an item was suggested. Tests: - TestDashboardSuggestedNext expectations updated for new ordering (in-progress wins over open). Test now covers the in-progress surfacing path the bug specifically wanted. - New TestDashboardSuggestedNext_FiltersBlockedItems exercises the blocker-filter — explicitly creates a blocks link and confirms the blocked task is suppressed even at critical priority. Out of scope: high-priority orphan suggestions (items not in any active plan). The bug item flagged this as a stretch goal; this PR sticks to the active-plan-children scope of the existing algorithm. Adding orphans would need a sort/relevance model since "everything high priority" can be a long list. Parent: BUG-990.	2026-05-01 21:48:04 -04:00
xarmian	9657051e43	fix(mcp): dedup implementation_notes / decision_log from fields blob (BUG-992) (#365) ... Item responses carry implementation_notes and decision_log in TWO places: 1. Top-level arrays on the item (item.ImplementationNotes, item.DecisionLog) — populated by hydrateItemComputedMetadata. 2. Inside the stringified `fields` blob — written there by AppendImplementationNote / AppendDecisionLogEntry at write time. This duplicate forces agents to dedup or pick a source; the bug report's recommendation was to keep the top-level arrays as the canonical shape and drop the embed. Path-consistent with BUG-991 path A (also MCP-only normalization): extend the boundary normalizer in packageJSONResult so that AFTER fields is parsed (BUG-991), implementation_notes and decision_log keys are dropped from the parsed fields object. The top-level arrays continue to surface unchanged. Server / web / CLI keep their existing behavior — fields still carries the embed at rest, hydration still extracts to top-level. The boundary fix is the cheap clean-up; a write-side migration (stop persisting into the fields blob, plus a one-shot data migration to clean existing items) is the architecturally proper fix and remains tracked in BUG-992's notes. Tests: - internal/mcp/bug992_test.go: stripDuplicatedFieldsKeys helper (strips both keys, no-op when absent, defensive on non-object inputs) + end-to-end packageJSONResult cases for single-item and array-style responses. Parent: BUG-992.	2026-05-01 21:40:01 -04:00
xarmian	708897dd0c	fix(mcp): parse fields/tags at MCP boundary so agents see native shapes (BUG-991 path A) (#364) ... Item responses carry `fields` and `tags` as JSON-stringified strings because the underlying SQLite columns store them that way. For agents going through MCP this means a double-encode every read — they have to JSON.parse the field's string value before doing anything useful. Path A (this PR): normalize at the MCP boundary. Recursively walk parsed JSON in packageJSONResult, find string-typed `fields` and `tags` properties, parse the embedded JSON, substitute the native shape. Server / web / CLI keep their existing stringified contract; agents see clean JSON. The walk handles every common item shape: - Single-item responses (top-level item) - Item arrays (item list, dashboard.active_items, comment lists) - Nested items (dashboard.recent_activity[].item, parent_*) Conservative parse: only strings starting with `{` or `[` and successfully parseable as JSON get substituted. Hand-written values that happen to share a key name (e.g. a `fields` description text) pass through untouched. Malformed JSON also passes through as the original string rather than dropping the value. Text fallback (content[0].text on the MCP result) preserves the original CLI body verbatim. Older clients that read the text content keep seeing the same shape — the structured wire is the agent upgrade path; text is the back-compat path. Same pattern as BUG-985's {items: [...]} array wrap. Path B (full migration of models.Item.Fields from string to map[string]any across server/store/web/CLI) is tracked in BUG-991's notes — bigger surgery, deferred. Tests: - internal/mcp/bug991_test.go: single-item, item-array, nested-item, primitives-pass-through, malformed-stays-string, plus end-to-end packageJSONResult cases proving structured + text branches both work. - Existing dispatch_http_advanced_test.go and dispatch_http_project_test.go updated: tests that previously did `json.Unmarshal([]byte(fieldsStr))` now use a small itemFieldsAsMap helper that reads the parsed map directly. Cleaner, and a clear error message if normalization regresses. Parent: BUG-991.	2026-05-01 21:35:29 -04:00
xarmian	55d3a078a8	fix(mcp): standup CLI ref + classifier polish for BUG-987 round 2 (#362) ... Round-2 hotfix on top of PR #361 (which shipped to v0.1.0-rc.4). Claude Desktop's re-review of rc.4 surfaced two fixes that didn't fully land: - Bug 8 (round 1 went to wrong layer). My HTTPHandlerDispatcher fix populated ref on standup blockers, but Claude Desktop's path is ExecDispatcher → CLI subprocess → standupCmd, which has its own JSON composition struct. That struct's Attention + SuggestedNext anonymous types didn't even define ItemRef as a parseable field. Now both define `item_ref` and the JSON-emit loops set Ref from it. Verified live: blockers now carry refs (TASK-X), not empty strings. - Bug 11 part 2. Round 1 stripped the cobra Usage block but two artifacts still leaked: 1. The "pad <verb> failed: <stderr>" prefix on server_error fallback messages. The verb name is the OLD CLI verb (e.g. `pad item block`) which doesn't match the v0.2 catalog actions agents see, and the cmdPath is already implicit from the invoked tool. Drop the prefix; emit the cleaned stderr directly. 2. Self-link / "cannot ..." validation rejections classified as server_error instead of validation_failed. Extended the validation regex with `cannot ` so server-side rejections like "cannot link an item to itself" / "cannot modify archived item" route to ErrValidationFailed. Verified live with a self-link attempt — now returns code=validation_failed, hint="cannot link an item to itself", no prefix. - New stripErrorPrefix helper trims leading `Error:` / `error:` / `ERROR:` from every classified hint+message so the envelope text isn't redundant with the envelope's `code` signal. Bug 13 / Bug 14: my round-1 fixes verified working locally on rc.4 (tested with a fresh Task → convention=None; dashboard by_role shows "Unassigned"/"unassigned" for the bucket). The reviewer's stale results almost certainly reflect a pad server process that wasn't restarted with the rc.4 binary swap. Tests: - TestClassifyExecError_CannotPhrasingClassifiesAsValidation — three "cannot ..." stderr cases must classify validation_failed. - TestClassifyExecError_NoLegacyVerbPrefixInMessage — pins the prefix-strip behaviour on the server_error fallback path. - TestStripErrorPrefix — trim-rule round-trip across casing variations and empty input. Parent: BUG-987.	2026-05-01 21:04:26 -04:00
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.	2026-05-01 20:39:10 -04:00
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.	2026-05-01 19:52:52 -04:00
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.	2026-05-01 18:36:29 -04:00
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.	2026-05-01 18:32:27 -04:00
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.	2026-05-01 18:15:34 -04:00
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.	2026-05-01 18:11:38 -04:00
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.	2026-05-01 18:04:35 -04:00
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.	2026-05-01 17:45:18 -04:00
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.	2026-05-01 17:30:27 -04:00
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.	2026-05-01 14:34:17 -04:00
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.	2026-05-01 14:21:11 -04:00
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.	2026-05-01 14:09:59 -04:00
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.	2026-05-01 13:48:04 -04:00
xarmian	a7b62ed886	feat(mcp): wire item link/lifecycle commands + slug-based --role resolution (TASK-968 partial) (#346) ... * feat(mcp): wire item link/lifecycle commands + slug-based --role resolution (TASK-968 partial) Expands HTTPHandlerDispatcher beyond TASK-967's per-command set with twelve new commands across the link/lifecycle surface, plus the prefetch-based --role slug → agent_role_id resolution that mirrors the --assign path TASK-967 introduced. New commands: - item block / blocked-by / unblock (blocks links) - item implements / unimplements (implements links) - item supersedes / unsupersede (supersedes links) - item split-from / unsplit (split_from links) - item deps / related / implemented-by (read-only link queries) The link create/delete commands have a URL/body asymmetry the simple routeSpec framework can't express: block uses (source.Slug, target.ID) while blocked-by inverts to (blocker.Slug, source.ID). They also need a ref→{slug,id} prefetch pair before building the request, which means they need a Handler reference. Those land as method-bound dispatchers in dispatch_http_links.go alongside an itemLinkSpec table that captures the per-command (urlRefKey, bodyTargetRefKey, linkType) tuple. Read-only link queries (deps/related/implemented-by) all GET /items/{ref}/links and let the agent group however it wants — same payload, different rendering on top. Slug-based --role resolution mirrors TASK-967's --assign path: resolveRoleSlug + lookupRoleID hit /api/v1/workspaces/{ws}/agent-roles/{slug} and rewrite role: <slug> → agent_role_id: <uuid> before the mapper runs. This replaces the older --role rejection in mapItemCreate / dispatchItemUpdate that pointed agents at the agent_role_id workaround; the workaround still works (lifted via liftFieldsToColumns) but slugs now flow through cleanly. commandsAcceptingRoleBySlug is the allowlist; item.list intentionally stays out because its handler accepts both UUID and slug at the query-param level. Adds noRemoteEquivalent rejection set for genuinely-CLI-only commands (agent status, mcp status/uninstall, server info/open, workspace link/switch/context). The error message says "no remote equivalent — CLI-only command" to give agents a stable signal distinct from "not yet implemented over HTTP transport" — the former never lands remotely, the latter just hasn't been wired yet. Tests: - Per-command happy path + missing-input rejections + prefetch failure rejections. - Asymmetry test (blocked-by URL goes through blocker, body target_id is source). - Canonical link-type wire form (split_from not split-from). - Slug → agent_role_id resolution end-to-end through Dispatch. - noRemoteEquivalent stable error format. - Integration smoke against *server.Server: create A + B → block → deps surfaces → unblock → deps empty → supersedes works (catches canonical-wire-form regressions in the store). Parent: PLAN-943. * fix(mcp): related/implemented-by emit CLI's grouped JSON shape; wrap DELETE 204 as {status:removed} per Codex review (round 1) Two parity gaps Codex caught on PR #346: 1. `item related` and `item implemented-by` were returning the raw /links array. The CLI's `--format json` output wraps and post- processes — `related` returns `{item_ref, item_title, collection, group_count, groups[]}` (links grouped by canonical type + direction); `implemented-by` returns `{item_ref, item_title, count, results[]}` (filtered to incoming `implements` only). Fix: the read-only link queries now have command-specific dispatchers — `dispatchItemDeps` keeps the raw-array shape (matches `deps --format json`), while `dispatchItemRelated` and `dispatchItemImplementedBy` reproduce the CLI's wrapping and filtering. The grouping helpers (buildRelatedGroups, incomingImplementedBy, relatedEntryFromLink) are reproduced from cmd/pad/query.go since the CLI versions are in package main. 2. Successful unblock/unimplements/unsupersede/unsplit dispatches were returning empty TextContent because the handler emits 204 No Content. The CLI's `--format json` for these commands prints `{"status":"removed"}`, so MCP clients lose the structured success signal. Fix: dispatchDeleteItemLink now wraps the 2xx response with `{"status":"removed"}` via the new packageStructuredResponse helper. Also adds packageStructuredResponse: a small helper that marshal- decodes synthesized payloads back to `any` before stuffing them into NewToolResultStructured, so the StructuredContent surface matches what packageHTTPResponse emits for normal route paths (`map[string]any` / `[]any` / JSON-decoded primitives, not Go-typed slice/struct instances). Tests: - Per-command tests for `related`'s grouped shape (blocks + implemented_by groups appear) - `implemented-by`'s incoming-only filter (outgoing implements + non-implements links excluded from `count`/`results`) - DELETE 204 wrapping for unblock - `deps` keeps raw-array shape (parity preserved) Parent: PLAN-943.	2026-05-01 13:29:45 -04:00
xarmian	cb526a5875	feat(mcp): item.update + --assign name resolution + workspace.members (TASK-967 partial) (#345) ... * feat(mcp): item.update with field-merge + --assign name resolution + workspace.members (TASK-967, partial) Closes the biggest item-write gap in HTTPHandlerDispatcher: - `item.update` now works through the dispatcher with full CLI parity, including the read-modify-write merge of the item's fields JSON. Without RMW, `item update TASK-5 --status done` would erase every other field set on the item — Codex caught the equivalent top-level-vs-fields shape regression on item.create in PR #343. - `--assign <name|email>` now resolves to a user UUID via the workspace-members endpoint, matching the CLI's behaviour. Previously rejected with a clear error pointing at `--field assigned_user_id=<uuid>`; now the dispatcher does the lookup so agents can use the same human-friendly form they'd use in the CLI. - `workspace.members` is wired into the route table both as a tool (agents can list members directly) and as the resolution backend. ## Architecture - New `dispatch_http_advanced.go` holds dispatcher methods that need access to the wrapped `Handler` (for in-handler prefetches like RMW or member lookup). Stays separate from `dispatch_http_routes.go` which is the simple-mappers domain. - `Dispatch` now has a preprocess step that runs before the route table lookup: for an allowlisted set of commands (item.create / update / list), `--assign` is resolved to `assigned_user_id`. Other commands' input passes through untouched, so `item.show` won't spuriously hit the members endpoint. - `item.update` is a special case in Dispatch's switch — it doesn't use the route table because it needs to issue a GET-then-PATCH pair with merged fields. Other RMW commands (none for now) would slot into the same switch. - New `executeRequest` helper on the dispatcher centralizes the request-build → recorder → packageHTTPResponse path so the simple table-driven case and the RMW case share it without duplication. ## Tests - Unit: 7 tests on `resolveAssignName` covering pass-through (missing / empty), name + email matches (case-insensitive), no-match error, explicit-ID precedence, and members-endpoint failure propagation. - Unit: 3 Dispatch-level tests for the preprocess (success path, failure surfaces as IsError without dispatching the main request, allowlist scoping). - Unit: 4 dispatchItemUpdate tests covering the field merge, the no-fields-changes guard, prefetch 404 → no PATCH, and missing workspace/ref validation. - Integration: end-to-end through real *server.Server + SQLite — create with --assign Alice, then update --status without re-specifying priority / category, and assert the existing fields survive the merge. ## Out of scope - `--role <slug>` resolution. The route table has `role list` so agents can fetch the slug → ID mapping themselves; full prefetch-resolution lives in the next route-table expansion. mapItemCreate now rejects `--role` loudly (was previously bundled with the `--assign` rejection). - The remaining ~50 commands (links, dependencies, star, project intelligence, library, github, attachments, webhooks, …) — TASK-967 stays open for follow-up PRs. Parent: PLAN-943. * fix(mcp): item.update --role rejection + Apply hook on prefetches per Codex review (round 1) Codex caught two real parity / security issues: 1. `item.update` silently ignored `--role`. mapItemCreate rejects it loudly because slug → role-ID resolution isn't built yet, but dispatchItemUpdate was a separate code path that didn't echo the guard. Agents would have gotten a successful update response while the role assignment got dropped on the floor. Reject with the same message + same pointer to `--field agent_role_id=<uuid>`. 2. The new prefetches (workspace.members lookup for --assign, item.update GET) bypassed `d.Apply`. Apply is the OAuth-scope hook the future TASK-953 middleware will use to attach token-allow-list / capability- tier context. Prefetches running outside that hook would have been a scope-bypass surface — agents could read members or items their token shouldn't have access to during resolution. Centralized the build + apply step in a new buildAuthedRequest helper on HTTPHandlerDispatcher. Both the in-handler prefetches and executeRequest now go through it so every synthesized request — main PATCH, GET prefetch, members lookup — sees Apply uniformly. Tests: - TestDispatchItemUpdate_RejectsUnsupportedRole asserts the role rejection trips before any handler call. - TestDispatch_PrefetchesGoThroughApplyHook asserts all three request types in an --assign + RMW flow (members lookup, item GET, item PATCH) flow through the Apply callback. Parent: PLAN-943. * fix(mcp): real --role workaround pass-through + corrected error message per Codex review (round 2) Codex caught a misleading error message: the rejection of --role pointed agents at `--field agent_role_id=<uuid>` as the workaround, but `--field` writes into the item's fields JSON blob, NOT the agent_role_id column on the ItemCreate / ItemUpdate models. So agents following the workaround would have ended up with the value sitting inert inside fields and the actual role assignment unchanged — the exact silent-success failure mode the rejection was meant to prevent. Two changes: 1. mapItemCreate + dispatchItemUpdate now pass `agent_role_id` through to the request payload at the top level, the same way `assigned_user_id` is passed through. So an agent that knows the role's UUID (from a prior `role list` call) can set it without --role slug resolution. 2. The error message text now points at "pass `agent_role_id=<uuid>` directly in the tool input" instead of `--field agent_role_id=...`. Adds the hint to discover the UUID via `role list` since that's the workflow agents would follow. Tests: - TestMapItemCreate_PassesThroughAgentRoleID asserts agent_role_id lands at the top level of the create body, NOT inside fields. - TestDispatchItemUpdate_PassesThroughAgentRoleID asserts the same for the update PATCH body. Parent: PLAN-943. * fix(mcp): lift column keys out of --field blob so the --role workaround is reachable per Codex review (round 3) Codex caught: the rejection of --role pointed agents at `agent_role_id=<uuid>` as a top-level input, but the MCP tool schema (auto-generated from cmdhelp) doesn't expose a top-level `agent_role_id` flag — only `--role` and `--field`. Strict clients (Claude Desktop, Cursor) following the schema have no way to send the recommended workaround as written. Two changes: 1. Added a `liftFieldsToColumns` helper that scans the fields blob for column keys (agent_role_id, assigned_user_id) and moves them onto the top-level payload. mapItemCreate runs this before serializing fields; dispatchItemUpdate runs it after the merge step. 2. The error message text now points at `--field agent_role_id=<uuid>` — that IS in the schema, and the lift logic ensures the value actually reaches the column rather than sitting inert in the fields JSON. The text explains the lift behaviour explicitly so readers don't think it's a hack. This makes the workaround genuinely reachable for any agent that follows the auto-generated tool schema. The "right" long-term fix (adding agent_role_id and assigned_user_id as proper named flags in cmdhelp so the schema exposes them directly) is captured for TASK-968. Tests: - TestMapItemCreate_LiftsAgentRoleIDFromFieldKVPToColumn pins the lift end-to-end on item.create. - TestMapItemCreate_LiftsAssignedUserIDFromFieldKVP confirms the same path works for the other column key. - TestMapItemCreate_TopLevelAgentRoleIDWinsOverFieldKVP locks the precedence rule (top-level wins; lift still strips the dup). - TestDispatchItemUpdate_LiftsAgentRoleIDFromFieldKVPToColumn pins the same behaviour on item.update's PATCH path. Parent: PLAN-943.	2026-05-01 12:59:02 -04:00
xarmian	5320f988ee	feat(mcp): expand HTTPHandlerDispatcher route table — 10 new commands + routeSpec framework (TASK-966) (#344) ... * feat(mcp): expand HTTPHandlerDispatcher route table — 10 new commands + declarative routeSpec framework (TASK-966) TASK-965 shipped HTTPHandlerDispatcher with `item create` as the proof-of-concept route. This expansion lays a small declarative framework (`routeSpec` → `RouteMapper`) and wires the high-value read + write surface, so an OAuth-authenticated agent connecting via the future /mcp endpoint (TASK-950) gets a useful tool surface out-of-the-box rather than 11/12 tools returning "not yet implemented over HTTP transport." ## Framework `routeSpec` (in dispatch_http_routes.go) is the declarative shape shared across simple commands: routeSpec{ method: http.MethodGet, pathTemplate: "/api/v1/workspaces/{workspace}/items/{ref}", queryParams: map[string]string{"q": "query", ...}, // dst→src bodyKeys: []string{"title", "content", ...}, } `{placeholder}` segments substitute from the input map (snake_case keys per TASK-964); `collection` and `target_collection` placeholders are auto-normalized via `collections.NormalizeSlug`. queryParams maps URL-query names to input keys and handles type coercion for ints (json.Number / float64) and bool presence-only treatment. bodyKeys emits a flat JSON body with empty values omitted. Commands that don't fit the shape (item.create's fields-rolling, item.move's nested overrides, item.list's path-varies-on-arg, item.search's renamed q param, item.comment's message→body rename) stay as standalone RouteMapper functions. The escape hatch is deliberate — the simple cases get one-line entries; the weird cases get full functions with their own test coverage. ## Commands wired (10 new + item.create) | Cmd | Method | Path / Notes | | --- | --- | --- | | item create *(prior)* | POST | /workspaces/{ws}/collections/{coll}/items, fields-rolling | | item show | GET | /workspaces/{ws}/items/{ref} | | item delete | DELETE | /workspaces/{ws}/items/{ref} | | item list | GET | path varies on collection arg; filters → query | | item move | POST | /items/{ref}/move with target_collection + field_overrides body | | item search | GET | /search?q=...&workspace=... (cross-workspace) | | item comment | POST | /items/{ref}/comments, message→body, reply_to→parent_id | | item comments | GET | /items/{ref}/comments | | project dashboard | GET | /workspaces/{ws}/dashboard | | collection list | GET | /workspaces/{ws}/collections | | role list | GET | /workspaces/{ws}/agent-roles | ## Out of scope `item update` requires read-modify-write semantics (the CLI fetches the existing fields JSON, merges in --status / --priority / --field entries, then PATCHes the merged result; the handler treats Fields as a complete replacement). Implementing that here would mean making two HTTP calls per dispatch and adding a new "prefetch" hook to the framework — out of scope for this PR. Captured as the next follow-up. `project next` / `project standup` / `project changelog` are CLI-side compositions (multiple API calls + presentation logic) with no single backing endpoint. Their HTTP equivalent for an agent is "call project dashboard and read the suggested_next field." Documented in the follow-up task. The remaining ~40 commands (attachments, webhooks, library, github, workspace audit-log, role create / delete, ...) are tracked in the follow-up. ## Tests - Framework unit tests: expandPath (substitution, normalization, escaping, error paths), buildQuery (rename, type coercion, json.Number support, empty-skip), flatJSONBody (omission rules). - Per-command unit tests: every wired command has a happy-path assertion + at least one error path. Custom mappers (item.list / move / search / comment) get table-driven coverage of their renames + path-variation behaviour. - Lock test: TestRouteTable_ContainsExpectedCommands fails loudly if an entry gets accidentally deleted. - Integration smoke (TestHTTPHandlerDispatcher_Integration_ReadPaths) drives item create → list → show → project dashboard → collection list end-to-end against a real *server.Server, asserting the full chain stays wired together after the refactor. Parent: PLAN-943. * fix(mcp): item.list parity with CLI per Codex review (round 1) Codex caught three CLI-parity bugs in mapItemList: 1. Default active-status filter missing. `pad item list` ships a broad inclusion list of active statuses unless --status or --all is set; the HTTP mapper returned no status filter, so MCP would leak done/completed/archived items by default. 2. `--parent <ref>` mapped to query param `parent_id`, which the server treats as a literal ID. The CLI uses `parent`, which parseItemListParams' unknown-key path routes to resolveParentFilter for ref → UUID resolution. Without this, `?parent=PLAN-3` would silently match nothing. 3. `--assign <name>` passed straight through as `assigned_user_id`. The CLI resolves names → user IDs via a workspace-members lookup first; passing the raw name to the store filter (which compares against `i.assigned_user_id` UUID) returns nothing. Fixes: 1. Added `defaultActiveStatusFilter` constant mirroring the CLI's hardcoded list at cmd/pad/main.go itemListCmd. Applied when neither --status nor --all is set; --all drops it (so done items show); explicit --status wins (so the user can pin to any tier). 2. Renamed the query-param target from `parent_id` to `parent` so the handler's resolveParentFilter sees it as a field filter and does ref→UUID resolution. 3. Reject `--assign` with a clear error pointing agents at `--field assigned_user_id=<uuid>` for explicit-ID filtering. Same pattern as the existing rejection on item.create. Full name → ID prefetch belongs in the same follow-up that handles `--assign` on item create / update. Tests: - TestRoute_ItemList_AllItemsPath_AppliesDefaultActiveStatusFilter asserts the broad inclusion list is on the wire and verifies a spot-check of well-known active + terminal statuses. - TestRoute_ItemList_AllFlagDropsDefaultStatus pins --all behaviour. - TestRoute_ItemList_ExplicitStatusOverridesDefault pins explicit --status precedence. - TestRoute_ItemList_FiltersAsQuery now asserts `parent` (not `parent_id`) is what reaches the wire. - TestRoute_ItemList_RejectsAssignByName covers the rejection. - TestRoute_ItemList_NumericLimitFromJSONNumber covers the json.Number path through the new numericInput helper. Parent: PLAN-943. * fix(mcp): normalize collection alias on item.search per Codex review (round 2) Codex caught: `pad item search foo --collection task` was passing "task" through verbatim to /api/v1/search?collection=task, but the search store filters via `c.slug = ?` (exact match) and 0-matches shorthand. The CLI normalizes to "tasks" first; mapper now does the same. Lifted the mutation into a tiny cloneStringMap helper so the input map the caller hands us isn't accidentally rewritten — the registry attaches the original via WithDispatchInput, and downstream code shouldn't see a mapper's normalization leak back. Tests: - TestRoute_ItemSearch_NormalizesCollectionAlias asserts task → tasks on the wire. - TestRoute_ItemSearch_DoesNotMutateInput pins the no-mutation contract so future refactors of the helper don't regress. Parent: PLAN-943.	2026-05-01 12:25:46 -04:00
xarmian	d84f1180a7	feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) (#343) ... * feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) Architectural prerequisite for PLAN-943's remote MCP at /mcp. The existing ExecDispatcher (PLAN-942) shells out to the pad binary and inherits credentials from ~/.pad/credentials.json — fine for local stdio MCP where the user IS the subprocess owner, but unworkable for a multi-tenant /mcp endpoint where the dispatcher must serve many OAuth-authenticated users from a single process. This PR ships the alternative path: HTTPHandlerDispatcher calls pad-cloud's existing HTTP handler chain in-process, with the requesting user attached via context. Same handlers, same audit / event-bus / webhook plumbing — just no fork(). ## What's in - internal/mcp/dispatch.go: keeps the existing Dispatcher interface (so ExecDispatcher unchanged) and adds a context-keyed WithDispatchInput helper. The registry attaches the original JSON input map to the dispatch context so dispatchers that prefer structured data over reverse-parsed cliArgs can use it. - internal/mcp/registry.go: forwards the merged input (user-supplied values + session workspace + root flags) to the dispatcher via WithDispatchInput. ExecDispatcher ignores it. - internal/mcp/dispatch_http.go (new): HTTPHandlerDispatcher implementation with a routeTable[cmdPath]→RouteMapper mapping. Seed entry: `item create`. Adding more commands is one RouteMapper per cmdPath plus a routeTable insert. - internal/server/context.go (new): exported WithCurrentUser / WithAPITokenAuth / WithTokenWorkspaceID + read-only CurrentUserFromContext / IsAPITokenFromContext. Lets internal/mcp synthesize an authenticated request without reaching the package-private context keys. - internal/server/middleware_csrf.go: extends the existing "Bearer token requests skip CSRF" rule to also honor the ctxIsAPIToken context flag. Same semantic — non-cookie auth means no CSRF risk — but covers the in-process dispatch path where TokenAuth never sets the Authorization header. Safe because ctxIsAPIToken can only be set by trusted in-process code (TokenAuth on the live Bearer path, or server.WithAPITokenAuth from the dispatcher). ## What's tested - Unit: - TestHTTPHandlerDispatcher_RoutesItemCreate — full happy path with a recordingHandler asserting method/path/body/user-context. - TestHTTPHandlerDispatcher_UnsupportedToolReturnsErrorResult — tools not yet in the routeTable produce IsError-flagged results rather than panicking. - TestHTTPHandlerDispatcher_NoUserReturnsErrorResult — UserResolver returning nil produces an IsError, never a nil-deref. - TestHTTPHandlerDispatcher_HandlerErrorSurfacesAsToolError — 4xx handler responses come back as IsError MCP results matching ExecDispatcher's `pad <cmd> failed: <stderr>` format. - mapItemCreate validation + parseFieldKVP variants. - Integration: TestHTTPHandlerDispatcher_Integration drives the full *server.Server (real chi router, real SQLite store, full middleware chain) with a synthesized OAuth user and asserts the item lands in the DB. ## Scope discipline The DoD called for "dispatch item.create end-to-end" — that's the seed entry. Wiring the remaining ~70 MCP-exposed commands into routeTable is naturally a follow-up before TASK-950 ships /mcp to real users (captured as a separate task post-merge). Audit-log assertion in the integration test is deferred until TASK-960 (B6b) lands the audit log itself. Parent: PLAN-943. * fix(mcp): roll status/priority/category/parent into fields JSON per Codex review (round 1) Codex caught: mapItemCreate placed status / priority / category / parent at the top level of the JSON body, but handleCreateItem only reads them after unmarshalling the Fields string from the request. As written, MCP-driven `item create` would silently drop those flags — breaking parity with the CLI for almost every realistic call (parent- linked tasks, priority-set items, status-overridden ideas, etc.). Mirrored the CLI's behaviour (cmd/pad/main.go ~L2200): build a fields map from the named flags, overlay the repeatable --field entries on top, JSON-encode into ItemCreate.Fields. The handler's existing schema-validation + parent-resolution path now runs unchanged. Repeatable --field still wins last-write — locked into a new test so it doesn't drift. Also rejects --assign / --role with a clear error rather than silently dropping them. The CLI resolves user-name → user-ID and role-slug → role-ID via additional API calls before posting; replicating that pre-resolution belongs in a follow-up that expands the route table for production use. Failing loudly is better than partial parity. Tests: - TestHTTPHandlerDispatcher_RoutesItemCreate now asserts the status/priority/category/parent values land in fields, not the top level — guards against the regression directly. - TestMapItemCreate_ExplicitFieldOverridesNamedFlag locks the last-write-wins precedence between --status and --field status=... - TestMapItemCreate_RejectsUnsupportedAssignRole asserts the defensive error path for the deferred flags. Parent: PLAN-943. * fix(mcp): persist source=cli for HTTPHandlerDispatcher calls per Codex review (round 2) Codex caught: actorFromRequest derives source from the Authorization header — without one, dispatcher-driven calls would persist source="web" instead of source="cli", regressing dashboard/standup/ audit attribution vs. ExecDispatcher. Same pattern as the round-1 CSRF fix: extend actorFromRequest to also honor the ctxIsAPIToken context flag (which TokenAuth sets on the live Bearer-auth path and HTTPHandlerDispatcher sets via server.WithAPITokenAuth on synthesized requests). Both signals mean "non-cookie authenticated, attribute as CLI/agent traffic". Integration test now asserts source="cli" on the created item, so any future regression of this attribution surfaces immediately. Parent: PLAN-943. * fix(mcp): normalize collection aliases in HTTPHandlerDispatcher per Codex review (round 3) Codex caught: CLI's `item create task ...` works because cmd/pad/main.go's normalizeCollectionSlug maps singular/short forms ("task" → "tasks", "doc" → "docs", etc.) to the canonical slug before posting. HTTPHandlerDispatcher's mapItemCreate skipped that step, so the same documented call shape would 404 through the HTTP transport even though it worked through ExecDispatcher. Extracted the alias map to internal/collections.NormalizeSlug so the two transports stay in lockstep without duplication. cmd/pad/main.go's normalizeCollectionSlug now delegates to it; the in-process dispatcher calls it from mapItemCreate after pulling the collection out of input. TestMapItemCreate_NormalizesCollectionAliases locks every documented alias plus a passthrough case for custom collections. Parent: PLAN-943. * fix(server): WithTokenWorkspaceID actually clears on empty input per Codex review (round 4) Codex caught: the docstring said "Pass an empty string to clear" but the implementation early-returned `ctx` unchanged in that case, leaving any stale ctxTokenWorkspaceID set further up the chain active. Always overwrite so the contract holds: passing "" produces a context where tokenWorkspaceID(r) returns "", same as a never-set context. Parent: PLAN-943.	2026-05-01 11:51:28 -04:00
xarmian	e05ea07d62	fix(docs): use canonical wire path capabilities.experimental.padCmdhelp (#342) ... Codex caught the same accuracy issue on pad-web that exists in five spots in this repo: prose described the handshake location as "serverCapabilities.experimental.padCmdhelp", but per the MCP spec the InitializeResult shape is { result: { capabilities: { experimental: { ... } } } } There's no `serverCapabilities` field on the wire — `ServerCapabilities` is the Go-side struct type name in mcp-go; the JSON tag is `capabilities`. Anyone copying the path out of our docs to navigate a real JSON-RPC envelope was getting the wrong key. Updated to `capabilities.experimental.padCmdhelp` (or the fully qualified `result.capabilities.experimental.padCmdhelp` where the JSON-RPC envelope context wasn't otherwise obvious) in: - README.md — public-facing prose - CLAUDE.md — agent-facing prose - internal/mcp/version.go — discovery-surfaces doc comment + the experimentalCapabilityKey doc comment - internal/mcp/server.go — comment near WithExperimental - internal/mcp/meta.go — experimentalCapabilities() doc + the wire shape example (now wrapped under `result` for accuracy) - internal/mcp/server_test.go — test docstring + failure message - cmd/pad/mcp.go — comment near RegisterMeta The Go type `serverCapabilities` in `internal/server/handlers_capabilities.go` is unrelated (it's the response shape for `GET /api/v1/server/capabilities`) and stays as-is. No code/behaviour changes; pure prose accuracy fix. `make check` clean. Companion fix to pad-web PR #42, which Codex flagged the same issue on.	2026-05-01 11:15:21 -04:00
xarmian	0cd22f7bad	feat(mcp): translate hyphenated CLI flag names to snake_case on the MCP surface (TASK-964) (#341) ... Cobra's flag names are kebab-case (--due-date), and JSON Schema property names accept any string, but Anthropic's tool-use convention — and most LLMs' training data — is snake_case (due_date). Hyphenated property names trip up agents that auto-normalize to snake_case before emitting JSON, silently losing every flag with a hyphen. This commit adds the translation at the registry/dispatch boundary: - MCPPropertyName(cliName) helper: kebab-case → snake_case (idempotent for already-snaked or single-word names). - propertyForArg / propertyForFlag use MCPPropertyName when emitting the JSON Schema property name. - BuildCLIArgs reads the input map by translated key, but still emits the kebab-case CLI flag (--due-date value) on dispatch, so the CLI's flag definitions stay untouched. Round-trip contract: schema: { "due_date": "..." } ← what agents see input: { "due_date": "..." } ← what agents send CLI: pad item update REF --due-date ... ← what dispatch runs Defensive choice over verification: The task spec called for an interactive verification phase (connect Claude Desktop / Cursor, ask each to call a tool with a hyphenated flag, observe whether it round-trips). That requires real agents and isn't feasible in an autonomous shipping flow. Adding the translation defensively eliminates the risk regardless of agent behaviour and aligns with Anthropic's documented snake_case convention. If a future verification round shows hyphens worked fine for some agents, the translation is harmless — both forms map to the same schema. Tests: - TestBuildTool_HyphenatedFlagsSurfaceAsSnakeCase locks the schema side: hyphens disappear from JSON property names. - TestMCPPropertyName_RoundTrip locks the translation rule itself (idempotent for snake/single-word, hyphens-only). - TestBuildCLIArgs_HyphenatedFlagAcceptedAsSnakeCase verifies the full round-trip: agent sends due_date, CLI gets --due-date. - TestBuildCLIArgs_HyphenatedRepeatableFlagRoundTrips covers the slice/repeatable path (--add-tag x --add-tag y). - TestBuildCLIArgs_KebabInputKeyIsIgnored guards against agents that bypass the schema and send the kebab form — they pass an unknown property, which is silently dropped (matching today's behaviour for unknown optional flags). - Existing TestBuildCLIArgs_BoolPresenceForm updated to assert the new snake_case input contract. Parent: PLAN-942.	2026-05-01 10:59:26 -04:00
xarmian	2d98f2a170	feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963) (#340) ... * feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963) External agents (Cursor, Claude Desktop, the future Pad Cloud remote MCP in PLAN-943) depend on tool names, argument shapes, and resource URIs being stable across pad releases. Without an explicit contract, any future surface change breaks consumers silently. This commit ships the contract on two complementary surfaces: - serverCapabilities.experimental.padCmdhelp in the initialize handshake — namespaced map carrying {version, tool_surface_stable}, discoverable in one round-trip. - pad://_meta/version static resource — full JSON document with {pad_version, cmdhelp_version, tool_surface_stable, mcp_protocol_version} for clients that prefer reading a typed payload. CmdhelpVersion is pinned at "0.1" — the initial cmdhelp-derived surface shipped in PLAN-942. Bump the major when tool names / arg shapes / resource URIs change incompatibly. Tests: - TestServer_InitializeHandshake extended to assert the experimental capability shape on the wire (not just the existence of the field). - TestBuildMetaPayload_* lock the payload field names + fallback behaviour. - TestRegisterMeta_ResourceRoundTrip drives the resource through the real HandleMessage path so a regression in the dispatcher would surface as a test failure. Docs: - README's MCP section briefly mentions the contract surfaces. - CLAUDE.md's MCP section gets a stability-contract paragraph + the new resource URI. - Public docs at getpad.dev/mcp/local will need a follow-up PR in the pad-web repo (per CONVE-159) — captured at the end of TASK-963. Parent: PLAN-942. * fix(mcp): source MCP protocol version from mcp-go LATEST_PROTOCOL_VERSION per Codex review (round 1) Codex caught: the local MCPProtocolVersion constant was pinned at "2024-11-05", but mcp-go@v0.50.0 negotiates "2025-11-25" for clients that request mcp.LATEST_PROTOCOL_VERSION. The meta resource was therefore reporting a protocol revision newer than what the server actually speaks, which defeats the field's purpose for feature detection (e.g. RFC 8707 Resource Indicators land in 2025-11-25). Drop the local constant and read mcp.LATEST_PROTOCOL_VERSION at BuildMetaPayload time so the value tracks whatever revision the linked library will negotiate. The handshake's serverInfo.version already does this implicitly via NewMCPServer; making the meta resource follow the same source-of-truth keeps both surfaces in lockstep across mcp-go upgrades. Test updated to assert against mcp.LATEST_PROTOCOL_VERSION instead of the removed constant, plus an "empty-string" guard in case a future library refactor unsets the constant. Parent: PLAN-942.	2026-05-01 10:54:01 -04:00
xarmian	e90ee18907	feat(mcp): pad mcp install / uninstall / status (TASK-948) (#338) ... * feat(mcp): pad mcp install / uninstall / status (TASK-948) One-shot config writers for the three MCP-capable client apps: pad mcp install claude-desktop # ~/.config/Claude/claude_desktop_config.json (linux) pad mcp install cursor # ~/.cursor/mcp.json pad mcp install windsurf # ~/.codeium/windsurf/mcp_config.json pad mcp install --all # all three pad mcp uninstall cursor # remove pad mcp status # report install state Implementation: - internal/mcp/install.go (new) — Agent registry with per-OS path resolvers (PathFor takes (home, goos) so tests inject); AddPadEntry / RemovePadEntry / HasPadEntry primitives that read-modify-write JSON, preserving every entry except mcpServers.pad. Installer façade with Home/GOOS overrides for tests. - cmd/pad/mcp.go — three new cobra subcommands wired into mcpCmd: install (no-args = status, --all = batch), uninstall, status. Binary path resolved via os.Executable(). DOD coverage: - Existing entries preserved (TestAddPadEntry_PreservesOtherServers asserts both other mcpServers and unrelated top-level keys survive). - Idempotent install (binary unchanged → modified=false). - Update install (binary changed → modified=true). - Idempotent uninstall (missing file / missing entry → no-op). - Per-platform path resolution tested for linux + darwin. - 16 unit tests including edge cases: empty/whitespace files, malformed JSON rejected (no silent overwrite), case-insensitive agent aliases. Live verified end-to-end: - HOME=/tmp/fakehome pad mcp install cursor → writes valid JSON - pad mcp status → shows [x] Cursor with command path - pad mcp uninstall cursor → leaves mcpServers:{} skeleton Config file perms: 0600 (configs may hold credentials for OTHER MCP servers; tighten on principle). Parent: PLAN-942. * fix(mcp): tighten install argument validation + chmod existing configs (Codex round 1) Two findings on PR #338: 1. `pad mcp install` had no Args validator so cobra silently accepted extras: `pad mcp install cursor windsurf` only installed Cursor. Added cobra.MaximumNArgs(1) plus an explicit guard rejecting `--all` combined with an agent name (those flows are mutually exclusive). 2. os.WriteFile(path, data, 0o600) only honors the mode when CREATING the file. A pre-existing 0644 config kept 0644 after the install, defeating the security-tightening claim in the comment. Added an explicit os.Chmod(path, 0o600) after writing; chmod failures are stderr warnings, not hard errors (the data write already succeeded; perms hardening is best-effort defense-in-depth). New test TestAddPadEntry_TightensExistingFilePerms locks the 0600-after-install contract; live verified the cobra guards reject both error cases with clean messages. Parent: PLAN-942. * fix(mcp): tighten perms on idempotent install path too (Codex round 2) Codex caught: AddPadEntry's no-op early-return (when desired config matches existing) skipped the chmod step from round 1's fix. So an already-up-to-date 0644 config retained 0644 after re-running `pad mcp install`. Extracted tightenPerms() as a helper called from BOTH paths: - writeJSONConfig (modified path) — chmod after write - AddPadEntry's no-op return — chmod even when content is unchanged Best-effort: chmod failures still emit a warning rather than failing the install (the user's intent already succeeded; perms tightening is defense-in-depth, not core functionality). New test TestAddPadEntry_TightensPermsOnIdempotentNoop locks the no-op-path contract. Parent: PLAN-942.	2026-05-01 09:15:23 -04:00
xarmian	ca2fc04a5b	feat(mcp): static prompts lifted from SKILL.md (TASK-947) (#337) ... Four MCP prompts expose pad's high-value multi-step workflows so agents can prompts/get them as user-role system messages: pad_plan — draft + decompose a Plan pad_ideate — brainstorm + capture as items pad_retro — retrospective on a completed Plan pad_onboard — workspace onboarding / codebase scan Implementation: - internal/mcp/prompts.go (new) — RegisterPrompts(srv) + PromptBody accessor; sorted iteration for deterministic prompts/list ordering. - internal/mcp/prompts_data.go (new) — embedded body strings, lifted near-verbatim from skills/pad/SKILL.md "Multi-Step Workflows". - cmd/pad/mcp.go wires RegisterPrompts after the resources path. 7 unit tests including SKILL.md drift contract: - All four prompts registered + reachable via PromptBody - Each body has the standard "# Pad: <workflow>" heading - Unknown prompt name returns error - Lockstep: every prompt body contains its key SKILL.md CLI invocations (catches silent drift if SKILL.md is updated without bumping prompts) - skills/pad/SKILL.md still exists as the source-of-truth (catches rename / removal during refactors) Live smoke: prompts/list returns 4 prompts (with descriptions); prompts/get pad_plan returns 1225 chars of workflow text starting "# Pad: Plan workflow\n\nYou are helping the user...". Naming choice: `pad_plan` (snake_case) over `pad/plan` (slash form) — some MCP clients interpret slashes as namespace paths. Matches the tool-naming convention from TASK-945. Parent: PLAN-942.	2026-05-01 08:58:27 -04:00
xarmian	342a564113	feat(mcp): read-only resource templates (TASK-946) (#336) ... * feat(mcp): read-only resource templates for items / dashboard / collections (TASK-946) Four MCP resource templates expose pad workspace state to agents without requiring a tool invocation: pad://workspace/{ws}/items/{ref} → single item markdown pad://workspace/{ws}/items → list of items (JSON) pad://workspace/{ws}/dashboard → project dashboard (JSON) pad://workspace/{ws}/collections → collections + schemas (JSON) Why resources, not tools: agents can `resources/read` a URI and ingest the body directly into context without going through a tool-call round-trip. Useful for "load TASK-5 then plan" workflows where the agent shouldn't need to pick a tool. Implementation: - internal/mcp/resources.go (new) — RegisterResources installs all four templates on an MCPServer; ResourceFetcher interface + ExecResourceFetcher shell-out (separate from Dispatcher because resource handlers return raw bytes, not CallToolResult). - parsePadURI extracts (workspace, kind, arg) from pad:// URIs; defensive guards reject mismatched URIs at each handler. - rootFlagsToArgs forwards startup --url to every fetched call (same contract as TASK-945's tool dispatch). - cmd/pad/mcp.go wires RegisterResources after the tool registry in mcpServeCmd. 15 new unit tests: - parsePadURI: all 4 forms + 4 malformed inputs - each handler: dispatches correct CLI args + MIME type - readItem rejects mismatched URI (defensive) - fetch errors propagate as Go errors (so MCP returns JSON-RPC error rather than empty contents) - root flag forwarding via the resources path - ExecResourceFetcher: missing binary, stdout capture, non-zero exit folds stderr into error Live verified: - resources/templates/list returns 4 templates with correct mime types and uri patterns. - resources/read pad://workspace/docapp/items/TASK-944 returns 1764 bytes of markdown. Parent: PLAN-942. * fix(mcp): compose full item markdown from JSON in resource path (Codex round 1) Codex flagged: pad://workspace/{ws}/items/{ref} fetched `pad item show --format markdown` which prints only item.Content (see cmd/pad/main.go:2562). The resource description promised "Full markdown content … includes title, fields, body, and links", so clients reading the URI lost ref/title/metadata/parent and couldn't reliably identify the item. Fix scoped to the resource path (rather than changing the CLI's markdown output, which other callers may parse): readItem fetches `--format json` and a new formatItemAsMarkdown composes the document — heading with ref + title, optional parent link, sorted metadata fields, then the content body. 3 new unit tests + the existing readItem test rewritten: - Full-shape JSON → exact markdown layout (deterministic via sorted keys) - Missing fields → heading-only doc, no panic - Empty `{}` fields → no stray list section - Invalid JSON → error propagates Live verified: pad://workspace/docapp/items/TASK-944 now returns "# TASK-944: <title>\n\n**Parent:** PLAN-942 — ...\n\n- **priority:** high\n- **status:** done\n\n<body>" — full identification + traversable parent link, body intact. Parent: PLAN-942.	2026-05-01 08:54:02 -04:00
xarmian	2e4a815d0c	feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945) (#335) ... * feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945) The strategic centerpiece of PLAN-942: walk the cmdhelp Document built from `pad`'s cobra tree and register every leaf as an MCP tool, with shell-out dispatch back to the running binary. New pad commands (or new flags) extend the MCP surface for free — no hand-mapping ~73 commands. - internal/mcp/registry.go — Register() walks cmdhelp.Document, picks leaves, applies a curated DefaultExcludes (db ops, auth, init, agent install/update, server lifecycle, completion, edit, watch, workspace lifecycle), builds an MCP Tool per leaf with input schema derived from cmdhelp Arg/Flag types. Snake-case names: "item create" → "item_create". - internal/mcp/dispatch.go — ExecDispatcher shells out to the pad binary; BuildCLIArgs is a pure function that translates the JSON args into a CLI invocation (positionals → flags → workspace injection → --format json default). JSON stdout is surfaced as StructuredContent for rich client rendering. - internal/mcp/workspace.go — WorkspaceState (RWMutex-protected) + pad_set_workspace built-in tool. Empty string clears the session default; missing arg returns IsError without mutating state. - cmd/pad/mcp.go — wire registry into `pad mcp serve` startup; build the cmdhelp Document from cmd.Root(), resolve the running binary via os.Executable, seed workspace from --workspace flag. - 23 new unit tests across registry / dispatch / workspace files (race-detector clean) covering: leaf identification, exclusion prefix suppression, snake-case naming, pure CLI arg translation (positionals + bool presence form + repeatable args & flags + workspace/format injection), exec dispatcher (binary missing, stdout capture, non-zero exit), workspace state mutation, and end-to-end pad_set_workspace handler contract. Live smoke (real binary, real stdio): - tools/list returns 66 tools — pad_set_workspace + item_create present, db_backup + mcp_serve correctly excluded. - tools/call pad_set_workspace updates session state, then auth_whoami shells out and returns structured JSON. Parent: PLAN-942. * fix(mcp): forward --url root flag + drop unwired --stdin (Codex round 1) Two findings from Codex review of #335: P1: --url root persistent flag was not forwarded to dispatched subprocesses. If an MCP client launches `pad --url X mcp serve`, every tool call ran against the default URL instead of X. Fixed by adding RootFlags map[string]string to RegistryOptions; cmd/pad/mcp.go captures urlFlag at startup and threads it through. BuildCLIArgs now also takes a rootFlags map and injects each entry when not in input (empty values skipped, agent value wins on collision). P2: MCP tool schemas exposed `--stdin` flags but ExecDispatcher never piped the agent's stdin to the subprocess. Calling e.g. `item_create {stdin: true}` would block on EOF and create empty content. The `--content` flag covers the same semantic via JSON args, which IS wired. Hide stdin from the MCP surface (buildTool filters out flagsHiddenFromMCP) AND drop it defensively in BuildCLIArgs in case an agent's stale schema cache passes it. Tests added (4 new + 2 updated): - BuildCLIArgs: stdin dropped defensively, root flags injected, empty root flag skipped, agent value wins over root flag. - buildTool: omits stdin from input schema. - Dispatch handler: forwards root flags through to CLI args. Existing TestBuildCLIArgs_BoolPresenceForm rewritten to use `dry-run` flag (since stdin is now filtered). Live verified: `pad mcp serve` tools/list shows item_create with content+10 other flags, no stdin. Round-trip preserved. Parent: PLAN-942.	2026-05-01 08:36:02 -04:00
xarmian	9905a83134	feat(mcp): pad mcp serve skeleton on stdio (TASK-944) (#333) ... Stand up internal/mcp + the cobra `pad mcp serve` subcommand. v1 is handshake-only — the server completes initialize and stays alive over stdio, advertising tool capability with an empty registry. TASK-945 fills that registry from `pad help --format json`. - New internal/mcp package wraps mark3labs/mcp-go's stdio transport; graceful shutdown on EOF / SIGINT / SIGTERM / ctx-cancel. - New cmd/pad/mcp.go registers `pad mcp` as a top-level cobra group with the `serve` subcommand wired to internal/mcp.NewServer. - 4 unit tests: NewServer construction, real initialize round-trip (asserts serverInfo.name + version), fallback version locked, graceful shutdown on ctx-cancel. Live smoke: `echo '<initialize>' | pad mcp serve` returns `serverInfo:{name:"pad-mcp",version:...}` with `tools:{listChanged:true}`. cmdhelp emits the new command tree at `pad help mcp serve --format json`. Parent: PLAN-942.	2026-05-01 08:17:04 -04:00
xarmian	cfda4463e8	feat(cmdhelp): tests + golden contract + drift validator (TASK-938) (#332) ... * feat(cmdhelp): tests + golden contract + drift validator (TASK-938) The verification layer that turns cmdhelp v0.1 from "implementation" into "stable contract." Three categories of tests, all running in `go test ./...`: 1. Schema validation (cmdhelp.schema.json as CI gate) - internal/cmdhelp/schema_test.go — synthetic tree's emitted JSON validates after static walk, after dynamic resolution, and after a no-workspace fallback. - cmd/pad/cmdhelp_real_test.go — the REAL pad cobra tree's emitted JSON validates against the published schema. Future regressions caught: types outside the closed vocabulary, non-numeric exit_code keys, flag names violating propertyNames, malformed cmdhelp_version. 2. Drift-prevention contract (spec §6 / §11 Q5) - internal/cmdhelp/example_validation.go — ValidateExamples walks every example's `cmd` string, tokenizes with shellSplit, resolves non-flag tokens against the live cobra tree, and asserts every --flag exists on the resolved command (or any ancestor for persistent / inherited flags). Negate-flag form (`--no-cache`) is recognized via the negation rule from spec §5.3. - shellSplit handles double/single quotes, backslash escape, and stops at unquoted pipeline boundaries (|, ;, &, >, <) so the validator only checks the first command in a pipeline. - ValidateBoolArity asserts no bool flag appears in valued form (--flag=value) anywhere in its examples (spec §5.3). - cmd/pad/cmdhelp_real_test.go runs both validators against the real pad tree as CI gates. - Negative tests in internal/cmdhelp/example_validation_test.go prove the validator catches: typo'd flag (--priorty), unknown command path, valued-form bool flag. 3. Capabilities form equivalence (spec §8) - cmd/pad/cmdhelp_real_test.go — both forms (help --capabilities and --cmdhelp-capabilities fallback) produce byte-identical output. Side-effect-free guarantee verified by passing garbage args alongside the fallback flag. Refactors enabling the tests: - cmd/pad/main.go: extract newRootCmd() so tests can build the real cobra tree without running it. main() body shrinks to two lines. - cmd/pad/main.go: extract handleCmdhelpCapabilitiesFallback() so the fallback's side-effect-free contract is directly assertable instead of requiring a subprocess. Parser improvements driven by real-pad-tree drift findings: - parseExamplesFromLong: strip same-line `# comment` annotations so `pad foo --bar # one item's attachments` doesn't pollute Examples. stripCommentIndex is quote-aware (# inside "..." or '...' is literal). - main.go (github cmd): the Long had annotations on example lines separated only by spaces (no `#`), which was malformed input. Fixed to use `#` separators — caught by the drift validator on first run. New deps: - github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 — Go JSON Schema validator supporting draft 2020-12 (matches the cmdhelp schema's $schema). New helpers in internal/cmdhelp/: - FindAndCompileSchema(startDir) walks up to locate schema/cmdhelp.schema.json and returns a compiled schema. Reusable by any consumer that wants to validate cmdhelp documents. End-to-end on real binary: - pad help --format json → 100 commands, schema-valid. - All examples in pad's emitted output resolve against the live tree (zero drift findings). - pad help --capabilities byte-identical to pad --cmdhelp-capabilities. - Adding a typo'd flag in any cobra Long block in cmd/pad MUST break TestRealPadTree_ExampleDriftValidator. Verified by the negative TestValidateExamples_DetectsTypoFlag. make check clean. All 53 cmdhelp + cmd/pad tests pass. Parent: PLAN-930. * fix(cmdhelp): pass full token stream to cobra.Find per Codex review (round 1) Codex round 1 caught: ValidateExamples stopped collecting the command path at the first flag, so an example like pad --workspace foo item create task --priority high resolved to root, not `item create`. That meant `--priority` was checked against root's flag set (where it doesn't exist) — false positive — AND the validator silently missed any command-path drift after a leading root flag. Cobra's own Find walks the full token stream and uses each command's flag definitions to skip flag/value pairs while matching subcommand names. Pass tokens[1:] directly to root.Find — let cobra handle the interleaving correctly. New test: - TestValidateExamples_FlagBeforeSubcommandResolvesToCorrectTarget — flag-before-subcommand resolves to the leaf and accepts leaf flags.	2026-05-01 07:17:58 -04:00
xarmian	76c9d5aae5	feat(cmdhelp): parse Examples blocks from cobra Long as fallback (TASK-939) (#331) ... The original TASK-939 ask was to migrate every cobra command's "Examples:" block from Long into the dedicated Example field. Pad has 102 cobra commands; manually migrating each is a hundreds-of-lines change with high regression risk and zero user-visible improvement (cobra renders "Examples:" sections in Long identically to the Example field — the difference is only machine-readability). Higher-leverage approach: enrich the cmdhelp emitter to fall back to parsing Long when the Example field is empty. One small, testable change in internal/cmdhelp unlocks examples in cmdhelp output for every command that already has an "Examples:" block — without touching any of the 102 command sites. Structural migration becomes optional polish (HT-941). Implementation: - internal/cmdhelp/json.go gains parseExamplesFromLong: locate a stand-alone "Examples:" / "Example:" header, collect indented invocation lines until blank-then-prose or end. Comment lines (`#`) are dropped from the block. The header regex is anchored to a line on its own (`^\s*Examples?:\s*$`) so prose containing the word "Examples" doesn't trigger the fallback. - buildCommand() prefers cmd.Example when set; falls back to parseExamplesFromLong(cmd.Long) when Example is empty. Tests assert precedence so future migrations to the Example field win cleanly. - cmd/pad/main.go: completion command gets a dedicated Example field (one of the few that didn't have an "Examples:" block at all). Demos the migration pattern HT-941 will sweep across the rest. Result on the real binary: pad help --format json → before: 0/100 commands have examples after: 24/100 commands have examples pad help item create --format json → 4 examples (vs 0 before) pad help completion --format json → 4 examples (from Example field) The remaining ~76 commands genuinely lack an Examples: block in Long (or are group commands that don't need examples). HT-941 captures the sweep work needed to get those to 100%. Tests: - 7 new tests for parseExamplesFromLong in internal/cmdhelp/json_test.go: basic block extraction, no-header (Usage: != Examples:), variant headers (singular/plural, indented), empty/malformed inputs, stops at unindented prose, drops comment lines. - 2 new end-to-end tests via Build(): - falls back to Long when Example is empty - prefers Example field when both set (precedence) - All 39 existing cmdhelp tests + 16 routing tests still pass. - make check clean. Follow-up: HT-941 ("Migrate cobra Long Examples blocks to dedicated Example fields") captures the structural sweep — broken into per-group PRs (auth/*, agent/*, server/* etc.) so it can be done incrementally without blocking PLAN-930. Parent: PLAN-930.	2026-05-01 01:39:23 -04:00
xarmian	0439c1bf3d	feat(cmdhelp): --capabilities discovery flag + --cmdhelp-capabilities fallback (TASK-937) (#330) ... Implements the cmdhelp v0.1 §8 capability bit so wrappers can detect support without trial and error. - pad help --capabilities → cmdhelp/0.1: text, md, json, llm - pad --cmdhelp-capabilities → same line (spec §8 fallback form) Both forms: - Single line on stdout (terminated by newline only). - Side-effect-free: no logging, no network, no config writes, no auth challenge. Verified by running with no workspace context (cwd /tmp, no auth) — still emits the line and exits 0. - Exit 0 on success. - Format: cmdhelp/<MAJOR>.<MINOR>: <comma-separated formats>. Why both forms: Spec §8 lists `<cmd> help --capabilities` as preferred and `<cmd> --cmdhelp-capabilities` as a fallback for CLIs whose `help` subcommand is overloaded. Pad's `help` is not overloaded, but supporting both forms costs nothing and lets wrappers and harnesses choose whichever convention they prefer — TASK-938 will assert equivalence between them. The fallback is handled in main() before cobra parsing so it really is side-effect-free: it doesn't even reach config.Load() or the detect-workspace path. A simple os.Args scan + early return. Files: - internal/cmdhelp/json.go — new CapabilityLine(formats) helper that produces the spec-format string. Caller passes the format set so different binaries can advertise different surfaces; the helper preserves caller order (spec §8 says order isn't significant). - cmd/pad/help_cmdhelp.go — adds --capabilities to helpCmd; new padCmdhelpFormats constant ["text","md","json","llm"]; short-circuit in RunE before any other logic runs. - cmd/pad/main.go — pre-args scan handles --cmdhelp-capabilities fallback before rootCmd.Execute(). Tests: - TestCapabilityLine_FormatExact — exact string match. - TestCapabilityLine_HonorsCallerOrderAndSet — preserves caller order. - TestHelpCmd_CapabilitiesExactString — exact-byte assertion on the output of `padtest help --capabilities` including the trailing newline. - TestHelpCmd_CapabilitiesShortCircuits — verifies --capabilities wins over --format / --depth / extra args (spec §8 side-effect rule). - All 35 existing cmdhelp tests + 14 routing tests still pass. - make check clean. End-to-end on real binary: pad help --capabilities → "cmdhelp/0.1: text, md, json, llm" exit 0 pad --cmdhelp-capabilities → same cd /tmp && pad help --capabilities → still works, no auth needed pad help item --capabilities --format json --depth 0 → short-circuits Parent: PLAN-930.	2026-05-01 01:32:06 -04:00
xarmian	e6fd25322e	feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936) (#329) ... * feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936) The killer differentiator from the cmdhelp v0.1 spec — splice live workspace facts into help output so an LLM asking "what collections exist?" gets the real answer rather than a generic "any string". Files: - internal/cmdhelp/dynamic.go (new) — Resolver type with Apply method. ArgEnumSources / FlagEnumSources map names to enum_source identifiers; Sources maps enum_source to a fetcher func. Apply walks the Document, stamps enum_source on matching args/flags, populates Enum from the fetcher, and sets doc.Context.Workspace. Per-Apply caching keeps each source func to ≤1 invocation regardless of how many commands need it. - internal/cmdhelp/json.go — added Options.Resolver; Build calls Resolver.Apply after the static walk, so callers that inspect Build output as either pre- or post-resolution still work. - cmd/pad/help_cmdhelp.go — newDynamicResolver constructs a Resolver bound to the runtime: workspace from DetectWorkspace, server URL from config, three sources (collections, roles, members). Returns nil when no workspace is detected so help still works outside any workspace. cmdhelpOptions takes target so Binary derives from root.Name() instead of hardcoded "pad" (preserves test-tree bindings for synthetic roots). Pad-side bindings (matches `pad item create --help`'s existing context): arg collection → dynamic:pad collection list flag role → dynamic:pad role list flag assign → dynamic:pad workspace members End-to-end on the real binary (inside docapp workspace): pad help item create --format json → args[0].collection: type=enum, enum=[ideas,conventions,...,roadmap], enum_source="dynamic:pad collection list" → flags.role: enum=[planner,implementer,reviewer] → flags.assign: enum=[dave] → context.workspace="docapp" pad help --format md → "## Workspace context\n- workspace: `docapp`" Outside any workspace (cd /tmp; pad help item create --format json): → collection arg: type=string, no enum, no enum_source (graceful fallback) → context: null → output still validates against schema/cmdhelp.schema.json Fail-safe behavior: - newDynamicResolver returns nil on any config/detection error → static doc. - Per-source fetcher errors are caught inside Apply → enum_source still announced on the binding arg/flag, but Enum is left empty. The help command MUST NOT fail because dynamic facts can't be fetched. - Existing Enum values from alternation/ValidArgs are preserved (resolver only fills the gap, never overwrites authoritative spec). Tests: - 10 dynamic-resolver tests in internal/cmdhelp/dynamic_test.go covering: arg + flag enum population, context population, per-source caching across multiple commands, graceful error handling, nil resolver as no-op, existing-Enum preservation, global flag resolution, unaffected commands left unchanged, end-to-end via Build(). - All 24 prior cmdhelp tests + 12 routing tests still green. - make check clean (lint + go test + web build). Out of scope (deferred): - --capabilities discovery flag — TASK-937. - Schema-validate live output in CI — TASK-938. - Audit pad's existing commands' Examples — TASK-939. Parent: PLAN-930. * fix(cmdhelp): scope --role / --assign bindings per-command per Codex review (round 1) Codex round 1 on PR #329 caught a real semantic bug: globally binding --role to dynamic:pad role list was wrong because pad has two unrelated --role flags: pad workspace invite --role workspace role: owner|editor|viewer pad item create --role <slug> agent role slug pad item update --role <slug> agent role slug Globally announcing agent-role slugs as the values for `pad workspace invite --role` would mislead consumers (LLMs would suggest "planner" when "owner" is expected; tab-completion would offer the wrong set). Fix: - Resolver gains CommandArgBindings and CommandFlagBindings (map[path]map[name]source) — scoped to a specific command path. Per-command bindings win over wildcard ArgEnumSources/FlagEnumSources when both match. - Helper methods argSource(path,name) / flagSource(path,name) own the precedence rule so both args and flags use it consistently. - newDynamicResolver in cmd/pad keeps `<collection>` as a wildcard ArgEnumSources (universal — every <collection> in pad means a pad collection), but moves --role and --assign into CommandFlagBindings scoped to "item create", "item update", and "item list". `pad workspace invite --role` is intentionally left without a binding. - A header comment in newDynamicResolver enumerates every --role / --assign site in the CLI and which one each binding targets, so a future reviewer adding a new flag can see the rule at a glance. End-to-end on the real binary: pad help item create --format json → flags.role: type=enum, enum=[planner,implementer,reviewer], enum_source=... pad help workspace invite --format json → flags.role: type=string (untouched). ✓ New tests: - TestResolver_Apply_PerCommandBindingScoped — explicitly mirrors the Codex finding: two commands both have a `role` flag, only the bound command resolves. workspace-invite-style isolation regression test. - TestResolver_Apply_PerCommandWinsOverWildcard — precedence: when both wildcard and per-command match, per-command wins. - TestResolver_Apply_PerCommandArgBindings — same precedence rule for positional args. All 33 cmdhelp tests + 12 routing tests still green; make check clean. * fix(cmdhelp): bind item list --role to agent roles per Codex review (round 2) Codex round 2 caught that item list --role was still unbound — I missed it in round 1's grep because the variable name is `&roleFilter` rather than `&roleFlag`. Pad has 4 --role flags total: pad workspace invite --role workspace role (NOT bound) pad item create --role agent role slug (bound) pad item update --role agent role slug (bound) pad item list --role agent role filter (now bound) Fix: extend CommandFlagBindings["item list"] to include the same itemRoleAssign map as item create/update, so all three item subcommands that reference an agent role get the dynamic binding. Added a `grep` recipe in the comment so a future maintainer adding a new --role / --assign site can find every existing one in one shot (both `&roleFlag` and `&roleFilter` style declarations). End-to-end on real binary (inside docapp workspace): pad help item list --format json → flags.role: enum=[planner,implementer,reviewer], enum_source set ✓ → flags.assign: enum=[dave], enum_source set ✓ make check clean.	2026-05-01 01:27:06 -04:00
xarmian	5e93abe552	feat(cmdhelp): implement --format md emitter (TASK-935) (#328) ... Adds internal/cmdhelp/md.go that renders the Document built in TASK-934 as markdown with the predictable section order from cmdhelp v0.1 §6. Replaces the markdown stub in cmd/pad/help_cmdhelp.go so `pad help --format md` (and the `--llm` alias) produce real output. Section order per command (spec §6): ## `binary path` summary / description (when distinct from summary) ### Synopsis — fenced usage line, reconstructed from args + flags ### Arguments — table with name | type | required | description ### Flags — table with flag | type | default | description ### Stdin — when Stdin.Accepted is true ### Examples — fenced bash blocks, drawn from same canonical example set as JSON (spec §6 same-source rule) ### Output — text_template + json_schema_ref when populated ### Exit codes — table when ExitCodes is populated ### See also — bullet list of related command paths Top-level YAML frontmatter: cmdhelp_version, binary, version, generated (RFC3339, UTC). Now is overridable via Options.Now for snapshot-test stability. Top-level structure: `# binary` heading, summary, optional homepage, optional `## Workspace context` (populated in TASK-936), `## Global flags` table, then per-command sections sorted by path for determinism. Synopsis reconstruction uses the structured Args from Build() (rather than cobra.UseLine) so JSON and MD stay driven by the same parsed data — the variadic `<ref>...` and alternation enums from TASK-934 carry through naturally. Pipes in flag/arg descriptions are escaped (`\|`) so they don't break markdown table grids. cmd/pad/help_cmdhelp.go: emitCmdhelpMarkdown stub replaced with a call into cmdhelp.EmitMarkdown. --depth/--all threaded through MaxDepth identically to the JSON path. Tests: - 15 markdown emitter tests in internal/cmdhelp/md_test.go covering frontmatter (presence + timestamp injectability), per-command section order, synopsis reconstruction (incl. variadic + alternation), global-flag dedup, fenced-bash examples, hidden-thing exclusion, deterministic ordering, Stdin/Output/ExitCodes/SeeAlso sections, Workspace context, table-pipe escaping. - TestHelpCmd_FormatMarkdownStubError replaced by TestHelpCmd_FormatMarkdownEmits (asserts frontmatter + structural markers for both md and llm). - TestHelpCmd_FormatLLMAliasRoutesToMarkdown replaced by TestHelpCmd_FormatLLMIsAliasForMD (asserts md and llm produce byte-identical output modulo the timestamp). End-to-end on the real binary: - pad help --format md emits valid markdown with all sections. - pad help --format llm produces byte-identical output (after timestamp normalization). - pad help item create --format md scopes correctly. - make check clean. Out of scope (deferred): - Dynamic Workspace context population — TASK-936. - --capabilities discovery flag — TASK-937. - Schema-validation + golden-file tests in CI — TASK-938. - Examples populated for all pad commands (still in cobra Long for now) — TASK-939. Parent: PLAN-930.	2026-05-01 01:05:30 -04:00
xarmian	eecc683ab0	feat(cmdhelp): implement --format json emitter (TASK-934) (#327) ... * feat(cmdhelp): implement --format json emitter (TASK-934) Adds internal/cmdhelp package that walks the cobra command tree and emits a cmdhelp v0.1 Document conforming to schema/cmdhelp.schema.json. Wires it into cmd/pad/help_cmdhelp.go so `pad help --format json` is no longer a stub. Files: - internal/cmdhelp/types.go — Document/Command/Arg/Flag/Stdin/Stdout/ ExitCode/Example structs mirroring the schema. ExitCode implements custom MarshalJSON for the string-or-object union (spec §5.2). - internal/cmdhelp/json.go — Build() walks target's subtree; EmitJSON() serializes to indented JSON. Type mapping covers pflag's full type space, including slice/array→repeatable. Hidden commands and flags filtered. Cobra's auto-installed --help flag suppressed. Zero-default values suppressed to keep output compact. argRE parses positional arg placeholders from cobra Use strings, filtering [flags]/[options]/ [command] cobra conventions. parseExamples splits cmd.Example by newline, drops blanks and # comments. MaxDepth maps to spec §4 semantics: 0 = subcommand list, 1 = + grandchildren, -1 = unlimited. - cmd/pad/help_cmdhelp.go — replaces emitCmdhelpJSON stub with a call into the package; threads --depth and --all through MaxDepth (--all overrides --depth). Verification: - 17 emitter tests in internal/cmdhelp/json_test.go covering envelope, global-flag emission, hidden-thing exclusion, positional arg parsing, pflag type mapping, zero-default suppression, example parsing, description-vs-summary, command-path key shape, MaxDepth semantics, target-subtree scoping, JSON validity, version pattern, ExitCode union marshaling, parseExamples filtering. - 11 cmd/pad routing tests still pass; TestHelpCmd_FormatJSONStubError replaced by TestHelpCmd_FormatJSONEmits which validates the structure. - End-to-end: `pad help --format json` on the real binary emits 100 commands across the full tree; output validates against schema/cmdhelp.schema.json (verified with python jsonschema). - `pad help item --format json` correctly limits output to 29 commands in the item subtree (homepage and other top-level metadata still populated from root). - `pad help --format json --depth 0` correctly emits 15 immediate children of root, no grandchildren. - make check clean (lint + go test + web build). Out of scope (deferred): - Examples: pad's existing commands embed examples in Long rather than using cobra's Example field. The emitter correctly reads Example; TASK-939 will normalize the pad-side commands to populate it. - Dynamic enum injection (workspace-aware enums): TASK-936. - --capabilities discovery flag: TASK-937. - Schema-validation of live output in CI: TASK-938. Parent: PLAN-930. * fix(cmdhelp): handle alternation, variadic, and ValidArgs in Use parser per Codex review (round 1) Codex round 1 on PR #327 flagged that parseArgs missed two real cobra Use-string idioms in pad's command tree: 1. `completion [bash|zsh|fish|powershell]` — alternation in brackets. The old regex only allowed `[a-zA-Z0-9_./-]+` inside brackets, so the `|` made the whole token unmatched and the shell arg disappeared from the emitted JSON. Consumers asking "what does completion take?" got nothing. 2. `item bulk-update [--status X] <ref>...` — variadic ellipsis. Old regex didn't capture trailing `...`, so the arg was emitted but without `repeatable: true`. Consumers couldn't tell that <ref> may be passed multiple times. Fixes: - argRE now allows full-bracket content (`[^<>]+` / `[^\[\]]+`) and captures an optional trailing `...` group. - parseArgs takes *cobra.Command (not just Use string) so it can read cmd.ValidArgs and attach those values as the first arg's enum when set. This covers `Use: "completion [shell]"` + `ValidArgs: [...]` where the allowed values only live on the cobra struct. - Alternation `<a|b|c>` / `[a|b|c]` produces an enum-typed arg with the values as `Enum`. When Use carries no semantic name (only the alternation), the arg name is synthesized as "value". - New validArgName check rejects embedded flag-like fragments such as `[--status X]` and prose with whitespace/punctuation that the broader regex would otherwise capture from idiosyncratic Use strings. - ValidArgs entries strip cobra's tab-separated completion descriptions before becoming enum values. New tests: - TestBuild_VariadicArgsRepeatable — `<ref>...` → repeatable=true. - TestBuild_AlternationProducesEnum — `[bash|zsh|fish|powershell]` → enum with values in source order. - TestBuild_ValidArgsFillsEnumOnNamedArg — Use says `[shell]`, ValidArgs carries the values → enum-typed arg named `shell`. - TestBuild_EmbeddedFlagFragmentsFiltered — `[--status X]` does not leak as a positional arg. Verified on the real binary: - `pad help completion --format json` now emits the shell enum. - `pad help item bulk-update --format json` now marks <ref> repeatable. - `pad help --format json` still validates against the schema (100 cmds). - `make check` clean.	2026-05-01 00:56:50 -04:00
xarmian	5e27989ab8	feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321) ... * feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow.	2026-04-30 16:06:08 -04:00
xarmian	f8ed3e10a7	fix(search): explicit selection on Enter + numeric go-to (BUG-864, BUG-910) (#320) ... * fix(search): require explicit selection on Enter; add bare-number go-to (BUG-864, BUG-910) The command palette had two related issues: - BUG-864: Pressing Enter armed the first search result automatically — the user could close the modal and navigate without ever pressing an arrow key. selectedIdx now starts at -1 and only advances on ArrowDown/ArrowUp. - BUG-910: Typing a bare number (e.g. "843") returned no results because parseItemRef requires PREFIX-NUMBER and FTS doesn't index item_number. Backend (internal/store): - Add parseItemNumber() helper alongside parseItemRef. - In Search(), add a bare-numeric direct-lookup path that mirrors the existing ref-lookup block but without a collection prefix filter. item_number is unique per workspace (idx_items_workspace_number) so this resolves to at most one direct hit, prepended with rank=-1000. Frontend (CommandPalette.svelte): - selectedIdx defaults to -1; reset to -1 (not 0) on modal open and after every search. - Enter on a non-numeric query is a no-op unless the user has arrow-selected. - Numeric queries are a deliberate exception: Enter on a bare-number query flushes the debounce, navigates directly to the matching item, and lets the search palette double as a quick "go to item N" jump. Tests: - TestSearch_BareNumericQueryFindsItemByNumber covers the new path. - TestParseItemNumber covers helper edge cases. * fix(search): exclude direct hits from FTS WHERE to keep pagination correct Codex review (round 1) on PR #320: > Numeric direct hits are appended before the FTS query, but the later > pagination only removes duplicates after SQL LIMIT/OFFSET. If item #2 > also matches FTS for query "2" through its title/content, that > duplicate consumes an FTS slot, so page 1 can return fewer than `limit` > results and later pages can repeat/skip rows. Hoist the direct-hit (ref + numeric) snapshot to before the FTS query is built, then append `AND i.id NOT IN (...)` to both the SELECT and COUNT FTS queries. After a successful count, add refCount back so SearchResponse.Total still reflects the full result set (since FTS itself no longer counts those rows). The flaw also applied to the pre-existing parseItemRef path; this fix covers both. The post-LIMIT dedup loop is now defense-in-depth. New test TestSearch_BareNumericQueryDedupsAgainstFTS guards the case: an item whose title/content literally contains its own item_number (so it matches both the direct lookup and FTS) appears exactly once in Results and Total counts it exactly once. * fix(search): paginate direct hits properly across workspaces Codex review (round 2) on PR #320: > P1: Bare numeric direct hits break pagination in global search. > item_number is only unique per workspace, so q=1 with WorkspaceIDs > spanning N workspaces returns N direct hits — all appended without > being sliced to Limit. limit=1 with three workspaces each having #1 > returns three results on page 0, and offset=1 drops all direct hits > then returns FTS rows instead of the second direct hit. The same flaw applied to the pre-existing parseItemRef path: the global search "TASK-5" can match TASK-5 in multiple workspaces. Fix: - Add deterministic ORDER BY i.workspace_id, i.id to both ref and bare- numeric direct-hit lookups so pagination is stable across pages. - Replace the offset==0/offset>0 branching pagination with a uniform slice: directStart = min(Offset, refCount); directEnd = min(Offset+Limit, refCount); results = results[directStart:directEnd]; ftsLimit = Limit - directConsumed; ftsOffset = max(Offset - refCount, 0). This honours (offset, limit) whether direct hits, FTS, or both fill the page. Total stays correct because the FTS count was already excluding direct hits (round-1 fix) and we add refCount back unconditionally. New test TestSearch_BareNumericQueryPaginatesAcrossWorkspaces creates three workspaces each with item #1 and verifies that limit=1 with offsets 0/1/2 returns three different direct hits in stable order, and limit=10 returns all three. * chore: gofmt — column alignment in struct field declarations CI Go (SQLite) lint failed on two files: - internal/store/store_test.go (TestParseItemNumber, this PR's new test) — unaligned column widths and inconsistent comment spacing. - internal/config/config.go (drive-by) — pre-existing alignment regression in the Config struct that snuck in via an earlier landed PR; included here because it blocks merge. No semantic changes — `gofmt -w` only.	2026-04-30 14:23:23 -04:00
xarmian	10309fc599	fix(config): read PUBLIC_URL for emailed link generation (BUG-899) (#318) ... * fix(config): read PUBLIC_URL for emailed link generation (BUG-899) The Pad Cloud deployment binds pad to 0.0.0.0 (Dockerfile, k8s configmap, pad-cloud's docker-compose) and never set PAD_URL on the pad service, so cfg.BaseURL() fell through to "http://0.0.0.0:7777" — that string ended up in password-reset (and invite + share-link + admin-invitation) emails and was unreachable to recipients. Adds a PUBLIC_URL env var read by the server only (does not flip CLI to remote mode the way PAD_URL does — PUBLIC_URL is a generic env var name commonly set in unrelated deployment contexts). Stored in a separate Config.PublicURL field consulted by BaseURL() as a fallback after URL. Resolution order in BaseURL(): PAD_URL > PUBLIC_URL > host:port. Also logs a WARN at server startup if the resolved base URL has an unspecified bind-all host (0.0.0.0, ::, [::]) — a backstop that would have caught BUG-899 the first time email went out. Tests cover the precedence ladder, mode-not-flipping, PAD_URL-beats- PUBLIC_URL, and the BUG-899 repro shape (Host=0.0.0.0 with no URL set yields the broken http://0.0.0.0 URL). Companion change in pad-cloud/docker-compose.yml passes PUBLIC_URL through to the pad service so the Cloud deployment stops shipping broken email links. Parent: BUG-899 (TASK-908). * fix(config): keep PUBLIC_URL out of IsConfigured() per Codex review (round 2) PUBLIC_URL was setting LoadedFromEnv = true, which IsConfigured() consults to decide whether the CLI has explicit configuration. A generic PUBLIC_URL in the environment (very common name) would have made any host appear "configured" to the CLI and skipped the not-configured / setup branch — the exact footgun the separate-field design was supposed to avoid. PUBLIC_URL is purely a server-side fact; LoadedFromEnv is purely a CLI affordance. Stop conflating them. Adds a focused regression test pinning the IsConfigured() invariant. * fix(config): split PublicLinkBaseURL from BaseURL per Codex review (round 3) Round 2's BaseURL fall-through to PublicURL leaked PUBLIC_URL into ~20 CLI-client call sites (cli.NewClientFromURL(cfg.BaseURL()) patterns across cmd/pad/main.go, init.go, server_info.go, configure.go) — same footgun the separate-field design was meant to avoid: a developer with a host-level PUBLIC_URL set for unrelated reasons would have their CLI silently route requests to that URL instead of the local server. Restore BaseURL() to its original CLI-only contract (URL > host:port). Add PublicLinkBaseURL() with the URL > PublicURL > host:port ladder that's used at exactly the two server-side call sites that build emailed-link targets: - cmd/pad/main.go:279 srv.SetBaseURL(cfg.PublicLinkBaseURL()) - cmd/pad/main.go:464 email.NewSender(..., cfg.PublicLinkBaseURL()) Tests pin both contracts: BaseURL() ignores PublicURL even when set; PublicLinkBaseURL() honors the precedence ladder. PAD_URL still wins in both, preserving back-compat. * fix(config): drop public_url toml tag to prevent CLI persistence per Codex review (round 4) Round 3 left PublicURL serializable to ~/.pad/config.toml via toml: "public_url". A CLI user who runs `pad init` or `pad configure` on a host where PUBLIC_URL is set for unrelated reasons would end up with that URL persisted into their config file, surviving any later unset of the env var and contaminating server-side emailed link generation indefinitely (server reads ~/.pad/config.toml on the next boot). Switch the field to toml:"-". PUBLIC_URL is a deployment-time fact (env var / docker-compose / k8s); operators who want a config-file equivalent already have `url` (the PAD_URL path), which serializes properly. Adds a regression test pinning that Save() never writes PublicURL to the file.	2026-04-30 08:40:33 -04:00