mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 22:10:25 +00:00
fix(mcp): remote pad_set_workspace embeds the workspace bootstrap (BUG-3326, ToolSurface 0.62) (#1710)
* fix(mcp): remote pad_set_workspace embeds the workspace bootstrap (BUG-3326) registerRemoteMCP passed no BootstrapFetcher, so over /mcp the tool returned only a not_persisted note: remote clients started sessions without the conventions and playbooks stdio clients get. HTTPResourceFetcher gains Bootstrap, delegating to its existing in-process bootstrap resource read, which runs as the token's user through the dispatcher's scope/consent perimeter. registerRemoteMCP builds the fetcher once and hands it to both Register and RegisterResources. Tests go through wireMCP over /mcp with PAT bearers: the embed is present for a member, and absent for a non-member while the owner of that workspace gets it. Co-Authored-By: Claude <noreply@anthropic.com> * chore(mcp): ToolSurfaceVersion 0.62 for the remote set_workspace bootstrap embed (BUG-3326) ADDITIVE bump (v0.53 / v0.28 grounds): remote pad_set_workspace gains the `bootstrap` member stdio always carried. No name, enum or param moved; the tools/list goldens are unchanged, the initialize goldens move only the version and the instructions header. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,115 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3326: over remote /mcp, pad_set_workspace embeds the workspace
|
||||
// bootstrap, as it always did over stdio. The server is built through
|
||||
// wireMCP (the production binding, CONVE-19), so a fetcher constructed but
|
||||
// not passed to the registry fails here.
|
||||
|
||||
// setWorkspaceOverRemote calls pad_set_workspace through /mcp with the
|
||||
// given PAT and returns the tool's decoded JSON payload.
|
||||
func (f *capFixture) setWorkspaceOverRemote(t *testing.T, pat, ws string) map[string]any {
|
||||
t.Helper()
|
||||
body := `{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"pad_set_workspace","arguments":{"workspace":"` + ws + `"}}}`
|
||||
req := capRoute{method: "POST", path: "/mcp", contentType: "application/json", body: body}
|
||||
rr := f.doWithBearer(t, req, pat)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("tools/call pad_set_workspace: status %d body %.300s", rr.Code, rr.Body.String())
|
||||
}
|
||||
raw := rr.Body.String()
|
||||
// The streamable transport may answer as an SSE frame; take its data line.
|
||||
if i := strings.Index(raw, "data: "); i >= 0 {
|
||||
raw = strings.TrimSpace(strings.SplitN(raw[i+len("data: "):], "\n", 2)[0])
|
||||
}
|
||||
var env struct {
|
||||
Result struct {
|
||||
IsError bool `json:"isError"`
|
||||
Content []struct {
|
||||
Text string `json:"text"`
|
||||
} `json:"content"`
|
||||
} `json:"result"`
|
||||
Error any `json:"error"`
|
||||
}
|
||||
if err := json.Unmarshal([]byte(raw), &env); err != nil {
|
||||
t.Fatalf("decode JSON-RPC response: %v (%.300s)", err, raw)
|
||||
}
|
||||
if env.Error != nil || env.Result.IsError || len(env.Result.Content) != 1 {
|
||||
t.Fatalf("pad_set_workspace did not succeed: %.500s", raw)
|
||||
}
|
||||
var out map[string]any
|
||||
if err := json.Unmarshal([]byte(env.Result.Content[0].Text), &out); err != nil {
|
||||
t.Fatalf("decode tool payload: %v (%.300s)", err, env.Result.Content[0].Text)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func (f *capFixture) doWithBearer(t *testing.T, rt capRoute, pat string) *httptest.ResponseRecorder {
|
||||
t.Helper()
|
||||
req := httptest.NewRequest(rt.method, rt.path, strings.NewReader(rt.body))
|
||||
req.Host = f.host
|
||||
req.RemoteAddr = "192.0.2.10:4242"
|
||||
req.Header.Set("Content-Type", rt.contentType)
|
||||
req.Header.Set("Accept", "application/json, text/event-stream")
|
||||
req.Header.Set("Authorization", "Bearer "+pat)
|
||||
rr := httptest.NewRecorder()
|
||||
f.srv.ServeHTTP(rr, req)
|
||||
return rr
|
||||
}
|
||||
|
||||
func bootstrapWorkspaceSlug(t *testing.T, out map[string]any) (string, bool) {
|
||||
t.Helper()
|
||||
bs, ok := out["bootstrap"].(map[string]any)
|
||||
if !ok {
|
||||
return "", false
|
||||
}
|
||||
ws, _ := bs["workspace"].(map[string]any)
|
||||
slug, _ := ws["slug"].(string)
|
||||
return slug, true
|
||||
}
|
||||
|
||||
func TestBUG3326_RemoteSetWorkspaceEmbedsBootstrap(t *testing.T) {
|
||||
f := newCapFixture(t, cfgHTTP, false, nil, "true")
|
||||
|
||||
out := f.setWorkspaceOverRemote(t, f.memberPAT, f.ws.Slug)
|
||||
if out["status"] != "not_persisted" {
|
||||
t.Fatalf("status = %v, want not_persisted (shared state): %v", out["status"], out)
|
||||
}
|
||||
slug, ok := bootstrapWorkspaceSlug(t, out)
|
||||
if !ok {
|
||||
t.Fatalf("remote pad_set_workspace carries no bootstrap: %v", out)
|
||||
}
|
||||
if slug != f.ws.Slug {
|
||||
t.Fatalf("bootstrap.workspace.slug = %q, want %q", slug, f.ws.Slug)
|
||||
}
|
||||
}
|
||||
|
||||
// The embed is read as the TOKEN's user: a workspace the caller is not a
|
||||
// member of embeds nothing, while its owner gets it (the control showing
|
||||
// the absence is scoping, not a broken fetch).
|
||||
func TestBUG3326_RemoteSetWorkspaceBootstrapIsScopedToCaller(t *testing.T) {
|
||||
f := newCapFixture(t, cfgHTTP, false, nil, "true")
|
||||
other, err := f.store.CreateWorkspace(models.WorkspaceCreate{Name: "Admin Only", Slug: "admin-only", OwnerID: f.admin.ID})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateWorkspace: %v", err)
|
||||
}
|
||||
if err := f.store.AddWorkspaceMember(other.ID, f.admin.ID, "owner"); err != nil {
|
||||
t.Fatalf("AddWorkspaceMember: %v", err)
|
||||
}
|
||||
|
||||
if slug, ok := bootstrapWorkspaceSlug(t, f.setWorkspaceOverRemote(t, f.adminPAT, other.Slug)); !ok || slug != other.Slug {
|
||||
t.Fatalf("control: the owner's call should embed %q's bootstrap, got ok=%v slug=%q", other.Slug, ok, slug)
|
||||
}
|
||||
out := f.setWorkspaceOverRemote(t, f.memberPAT, other.Slug)
|
||||
if _, ok := out["bootstrap"]; ok {
|
||||
t.Fatalf("a non-member's pad_set_workspace embedded another workspace's bootstrap: %v", out)
|
||||
}
|
||||
}
|
||||
@@ -1333,6 +1333,10 @@ func newObservedEventBus(cfg *config.Config, rc *redis.Client, redisKeys redisns
|
||||
// build the remote server through this same function rather than
|
||||
// assembling its own, which would vouch for the test instead of this binding.
|
||||
func registerRemoteMCP(mcpSrv *mcpserver.Server, mcpDoc *cmdhelp.Document, dispatcher *mcpserver.HTTPHandlerDispatcher) error {
|
||||
// One in-process fetcher serves both the resources and
|
||||
// pad_set_workspace's bootstrap embed (BUG-3326), so the embed reads
|
||||
// through the same per-user auth/consent perimeter as the resource.
|
||||
resourceFetcher := mcpserver.NewHTTPResourceFetcher(dispatcher)
|
||||
if _, regErr := mcpserver.Register(mcpSrv.MCP(), mcpserver.RegistryOptions{
|
||||
Doc: mcpDoc,
|
||||
// Shared multi-user state: this one stateless process
|
||||
@@ -1345,9 +1349,10 @@ func registerRemoteMCP(mcpSrv *mcpserver.Server, mcpDoc *cmdhelp.Document, dispa
|
||||
// maybeInjectWorkspace default). Local `pad mcp serve`
|
||||
// (cmd/pad/mcp.go) keeps NewWorkspaceState — it's
|
||||
// single-user-per-process and safe to inject.
|
||||
Workspace: mcpserver.NewSharedWorkspaceState(),
|
||||
Dispatcher: dispatcher,
|
||||
PadVersion: fullVersion(),
|
||||
Workspace: mcpserver.NewSharedWorkspaceState(),
|
||||
Dispatcher: dispatcher,
|
||||
BootstrapFetcher: resourceFetcher,
|
||||
PadVersion: fullVersion(),
|
||||
}); regErr != nil {
|
||||
return fmt.Errorf("register MCP catalog: %w", regErr)
|
||||
}
|
||||
@@ -1367,7 +1372,7 @@ func registerRemoteMCP(mcpSrv *mcpserver.Server, mcpDoc *cmdhelp.Document, dispa
|
||||
// available over remote /mcp.
|
||||
mcpserver.RegisterResources(
|
||||
mcpSrv.MCP(),
|
||||
mcpserver.NewHTTPResourceFetcher(dispatcher),
|
||||
resourceFetcher,
|
||||
nil, // no root flags on the remote transport (no --url)
|
||||
)
|
||||
return nil
|
||||
|
||||
+2
-2
File diff suppressed because one or more lines are too long
+2
-2
File diff suppressed because one or more lines are too long
@@ -6,7 +6,7 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl
|
||||
|
||||
If the user is asking general code questions with no project-management thread, you don't need this server.
|
||||
|
||||
## Tool surface (v0.61)
|
||||
## Tool surface (v0.62)
|
||||
|
||||
Ten resource × action tools, plus `pad_set_workspace`. Eleven tools total.
|
||||
|
||||
|
||||
@@ -111,6 +111,22 @@ func (f *HTTPResourceFetcher) Fetch(ctx context.Context, args []string) (string,
|
||||
}
|
||||
}
|
||||
|
||||
// Bootstrap satisfies BootstrapFetcher, so the remote transport's
|
||||
// pad_set_workspace embeds the same blob its bootstrap resource serves
|
||||
// (BUG-3326). It is the resource read itself, run as the dispatcher's
|
||||
// resolved user through the same auth/consent perimeter, so the embed can
|
||||
// never reach a workspace the token cannot read.
|
||||
func (f *HTTPResourceFetcher) Bootstrap(ctx context.Context, workspace string) ([]byte, error) {
|
||||
if workspace == "" {
|
||||
return nil, fmt.Errorf("bootstrap fetcher: workspace is required")
|
||||
}
|
||||
body, err := f.Fetch(ctx, []string{"bootstrap", "--workspace", workspace, "--format", "json"})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return []byte(body), nil
|
||||
}
|
||||
|
||||
// FetchBytes serves the attachment-download resource read in-process,
|
||||
// bounding retained output at the resource limit so a large blob can't
|
||||
// balloon memory in the shared cloud process (the stdout-cap analog of
|
||||
|
||||
+13
-1
@@ -1726,6 +1726,18 @@ const CmdhelpVersion = "0.1"
|
||||
// unchanged. A client that confirms destructive tools will now
|
||||
// confirm pad_workspace calls.
|
||||
//
|
||||
// 0.62 — BUG-3326. ADDITIVE bump on the v0.53 / v0.28 grounds: no
|
||||
// tool name, action enum or param shape moved. Over REMOTE /mcp,
|
||||
// `pad_set_workspace` now embeds the workspace bootstrap under
|
||||
// `bootstrap`, as the stdio transport always did and as the tool's
|
||||
// description already promised: registerRemoteMCP passed no
|
||||
// BootstrapFetcher, so the response carried only {workspace, status:
|
||||
// not_persisted, note}. The embed is the bootstrap resource's own
|
||||
// in-process read (HTTPResourceFetcher), run as the token's user
|
||||
// through the dispatcher's scope/consent perimeter, so a workspace the
|
||||
// caller cannot read embeds nothing; a failed fetch stays non-fatal,
|
||||
// as on stdio. Stdio is unchanged.
|
||||
//
|
||||
// Post-0.37, deliberately NO bump (BUG-3098): `workspace_member_limit`
|
||||
// joins the structured 403 allow-list, as ErrWorkspaceMemberLimit with
|
||||
// its own hint (not the plan-limit "upgrade" hint). The server emits it
|
||||
@@ -1734,7 +1746,7 @@ const CmdhelpVersion = "0.1"
|
||||
// this surface can receive it; the entry exists so a future action does
|
||||
// not collapse it to permission_denied. When an action that can reach
|
||||
// it is added, that addition is the contract change and owns the bump.
|
||||
const ToolSurfaceVersion = "0.61"
|
||||
const ToolSurfaceVersion = "0.62"
|
||||
|
||||
// MetaVersionURI is the canonical URI of the queryable version document.
|
||||
// Lives outside the pad://workspace/{ws}/... namespace because it's a
|
||||
|
||||
@@ -114,10 +114,11 @@ func (s *WorkspaceState) ResolveDefault() string {
|
||||
//
|
||||
// bootstrapFetcher is optional. When non-nil, the response embeds the
|
||||
// AgentBootstrap JSON under `bootstrap`, so a single set-workspace call
|
||||
// hands the agent a fully-loaded session. When nil (e.g. early in
|
||||
// pad-cloud's HTTP dispatch where no CLI is available yet), the
|
||||
// response shape stays {workspace, status} and clients can fetch
|
||||
// bootstrap separately via pad_meta.action=bootstrap or the resource.
|
||||
// hands the agent a fully-loaded session. Both production transports
|
||||
// wire one: stdio an ExecBootstrapFetcher, remote /mcp its in-process
|
||||
// HTTPResourceFetcher (BUG-3326). When nil, the response carries no
|
||||
// `bootstrap` and clients can fetch it separately via
|
||||
// pad_meta.action=bootstrap or the resource.
|
||||
func SetWorkspaceTool(state *WorkspaceState, bootstrapFetcher BootstrapFetcher) (mcp.Tool, server.ToolHandlerFunc) {
|
||||
// The tool BEHAVES differently per deployment, so the description must
|
||||
// too (BUG-1865): a single-user local server persists the session
|
||||
|
||||
Reference in New Issue
Block a user