diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index e33a7d98..09bc8ced 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -116,6 +116,41 @@ var commandsAcceptingAssignByName = map[string]struct{}{ "item list": {}, } +// commandsAcceptingRoleBySlug is the allowlist of cmdPaths whose +// `--role ` input should be resolved to an agent_role_id UUID +// via the agent-roles endpoint before the mapper sees it. Symmetric +// to commandsAcceptingAssignByName. +// +// `item list` is intentionally NOT in this set: the LIST handler +// accepts both UUID and slug at the query-param level (the store's +// list path forks on UUID-vs-slug), so resolving slugs there would +// add an unnecessary hop without any behavioural change. Only +// commands whose handlers require the canonical UUID are listed. +var commandsAcceptingRoleBySlug = map[string]struct{}{ + "item create": {}, + "item update": {}, +} + +// noRemoteEquivalent enumerates leaf commands that have no useful +// HTTP-transport mapping because they mutate or inspect local state +// only — config files, MCP-client mcp.json entries, the local server +// process. Distinct from "not yet implemented over HTTP transport" +// because those will eventually land; these never will. +// +// Surfacing the distinction lets agents recognize-and-skip rather +// than retrying or escalating. The error message is stable so +// downstream tooling can match on it if needed. +var noRemoteEquivalent = map[string]struct{}{ + "agent status": {}, // local skill-detection (~/.claude/, etc.) + "mcp status": {}, // local mcp.json across MCP clients + "mcp uninstall": {}, // local mcp.json mutation + "server info": {}, // local pad-server process state + "server open": {}, // local browser launch + "workspace link": {}, // local .pad.toml mutation + "workspace switch": {}, // local .pad.toml mutation + "workspace context": {}, // local .pad.toml inspection +} + // Dispatch satisfies the Dispatcher interface. cliArgs are accepted // for interface compatibility but ignored — HTTPHandlerDispatcher // reads the structured input attached by the registry via @@ -145,6 +180,20 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin } cmdKey := strings.Join(cmdPath, " ") + + // Reject CLI-only commands up front with a stable, agent-recognizable + // message. These never get an HTTP mapping — the action is + // inherently local-state-only — so failing fast saves agents the + // retry-or-escalate cycle they'd run on a "not yet implemented" + // reply. + if _, local := noRemoteEquivalent[cmdKey]; local { + return mcp.NewToolResultErrorf( + "%s: no remote equivalent — CLI-only command "+ + "(operates on local pad client / config state, not the workspace)", + cmdKey, + ), nil + } + user := d.UserResolver(ctx) if user == nil { return mcp.NewToolResultErrorf("%s: no authenticated user in context", cmdKey), nil @@ -169,6 +218,20 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin } } + // Preprocess input: resolve --role slug → agent_role_id for + // commands that accept the shorthand. Symmetric to the --assign + // preprocess above. Failure here surfaces as IsError so agents + // see the resolution error instead of a silently dropped role + // assignment (the regression the older mapper-level rejection was + // guarding against — Codex review #345 round 1). + if _, ok := commandsAcceptingRoleBySlug[cmdKey]; ok { + var err error + input, err = d.resolveRoleSlug(ctx, user, input) + if err != nil { + return mcp.NewToolResultErrorf("%s: resolve --role: %s", cmdKey, err.Error()), nil + } + } + // Special-case routes that need read-modify-write or other // in-handler prefetches. These live as methods on the dispatcher // because they need the Handler reference; the simple route @@ -176,6 +239,26 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin switch cmdKey { case "item update": return d.dispatchItemUpdate(ctx, input, user) + case "item deps": + return d.dispatchItemDeps(ctx, input, user) + case "item related": + return d.dispatchItemRelated(ctx, input, user) + case "item implemented-by": + return d.dispatchItemImplementedBy(ctx, input, user) + } + + // Item link create/delete commands. The asymmetry between which + // arg drives the URL slug and which drives the body's target_id + // (block: source→URL, target→body; blocked-by: blocker→URL, + // source→body) lives in itemLinkSpecs; the dispatcher just looks + // up the spec and forwards. + if spec, ok := itemLinkSpecs[cmdKey]; ok { + switch cmdKey { + case "item unblock", "item unimplements", "item unsupersede", "item unsplit": + return d.dispatchDeleteItemLink(ctx, input, user, spec) + default: + return d.dispatchCreateItemLink(ctx, input, user, spec) + } } routes := d.Routes @@ -430,26 +513,22 @@ func mapItemCreate(input map[string]any) (method, path string, body []byte, err if v, ok := input["agent_role_id"].(string); ok && v != "" { payload["agent_role_id"] = v } - // `--role` slug → role-ID resolution belongs in the next - // route-table expansion alongside the other prefetch-based - // resolutions. For now, reject loudly so agents don't silently - // lose the role assignment. The workaround the error message - // points at — passing `agent_role_id=` directly in the - // tool input — is genuinely supported (see the pass-through - // above); `--field agent_role_id=...` is NOT a workaround - // because that writes into the fields JSON blob, not the - // agent_role_id column. (Codex review #345 round 2.) - if v, ok := input["role"]; ok { - if s, ok := v.(string); ok && s != "" { - return "", "", nil, fmt.Errorf( - "--role is not yet supported by HTTPHandlerDispatcher; " + - "slug → role-ID resolution lands in the next route-table " + - "expansion. For now, pass `--field agent_role_id=` — " + - "the dispatcher recognizes column keys from --field and " + - "writes them to the column rather than the fields blob. " + - "Use `role list` to find the UUID.") - } - } + // `--role` is now resolved at the dispatcher level (TASK-968): + // Dispatch's preprocess step rewrites the slug to the canonical + // `agent_role_id` UUID via the agent-roles endpoint before the + // mapper runs. By the time we get here either: + // + // - input had no `role` key — pass-through above already + // handled an explicit `agent_role_id`. + // - input had `role: ` and resolution succeeded — the + // preprocess set agent_role_id and removed `role`, again + // handled by the pass-through above. + // - resolution failed — Dispatch returned IsError before + // calling the mapper, so we don't reach this point. + // + // The mapper just trusts the resolved input. The pass-through + // for explicit `agent_role_id` (above) is the only role-related + // branch that needs to live in the mapper itself. body, err = json.Marshal(payload) if err != nil { diff --git a/internal/mcp/dispatch_http_advanced.go b/internal/mcp/dispatch_http_advanced.go index 332a496f..9eef08a9 100644 --- a/internal/mcp/dispatch_http_advanced.go +++ b/internal/mcp/dispatch_http_advanced.go @@ -67,6 +67,116 @@ func (d *HTTPHandlerDispatcher) resolveAssignName( return out, nil } +// resolveRoleSlug rewrites a `--role ` input into +// `agent_role_id ` by hitting the agent-roles endpoint and +// finding a matching role. Mirrors the CLI's behaviour in +// itemCreateCmd / itemUpdateCmd which treats `--role` as a slug or +// ID and resolves to the column UUID before sending the create/ +// update — without resolution, agents passing slugs would silently +// get empty results (the store filters by `i.agent_role_id = ?` +// UUID, with slug accepted only on the LIST endpoint, not the +// item-mutation handlers). +// +// Symmetric to resolveAssignName: returns the input map with `role` +// replaced by `agent_role_id` when a match is found, or unchanged +// when `role` is missing / empty. Mismatches return a clear error. +// +// The handleGetAgentRole endpoint at /agent-roles/{roleID} accepts +// either a UUID or a slug as roleID, so this single GET resolves +// both. If the caller passed an explicit `agent_role_id` alongside +// `--role`, the explicit ID wins (matches the --assign precedence +// in resolveAssignName). +// +// The returned map is always a fresh copy — the caller's reference +// isn't mutated. +func (d *HTTPHandlerDispatcher) resolveRoleSlug( + ctx context.Context, + user *models.User, + input map[string]any, +) (map[string]any, error) { + rawRole, present := input["role"] + if !present { + return input, nil + } + role, _ := rawRole.(string) + if role == "" { + return input, nil + } + out := cloneStringMap(input) + if existingID, _ := out["agent_role_id"].(string); existingID != "" { + // Explicit ID wins over slug; drop the role key to avoid the + // resolution lookup below. + delete(out, "role") + return out, nil + } + + workspace, _ := input["workspace"].(string) + if workspace == "" { + return nil, fmt.Errorf("workspace is required to resolve --role") + } + + roleID, err := d.lookupRoleID(ctx, user, workspace, role) + if err != nil { + return nil, err + } + out["agent_role_id"] = roleID + delete(out, "role") + return out, nil +} + +// lookupRoleID issues an in-handler GET against +// /api/v1/workspaces/{ws}/agent-roles/{slug} and returns the role's +// canonical id. The handler accepts either UUID or slug for roleID +// (see handleGetAgentRole), so callers can pass a slug like +// "implementer" or a pre-resolved UUID interchangeably. +// +// Goes through buildAuthedRequest so d.Apply (the OAuth-scope hook) +// sees this prefetch the same as a top-level dispatch — no scope +// bypass during role resolution. +// +// Errors: +// +// - underlying handler returns 404 → "no agent role matches --role %q" +// (clearer than the raw 404 body for agents). +// - other non-2xx → wrapped error with body. +// - response shape doesn't include id → error. +func (d *HTTPHandlerDispatcher) lookupRoleID( + ctx context.Context, + user *models.User, + workspace string, + role string, +) (string, error) { + path := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/agent-roles/" + url.PathEscape(role) + req, err := d.buildAuthedRequest(ctx, http.MethodGet, path, nil, user) + if err != nil { + return "", fmt.Errorf("build agent-role request: %w", err) + } + rec := httptest.NewRecorder() + d.Handler.ServeHTTP(rec, req) + if rec.Code == http.StatusNotFound { + return "", fmt.Errorf("no agent role matches --role %q", role) + } + if rec.Code >= 400 { + body := strings.TrimSpace(rec.Body.String()) + if body == "" { + body = http.StatusText(rec.Code) + } + return "", fmt.Errorf("look up agent role: %d %s", rec.Code, body) + } + + var resp struct { + ID string `json:"id"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + return "", fmt.Errorf("parse agent-role response: %w", err) + } + if resp.ID == "" { + return "", fmt.Errorf("agent-role response missing id for %q", role) + } + return resp.ID, nil +} + // lookupAssigneeID issues an in-handler GET against // /api/v1/workspaces/{ws}/members and returns the user_id whose // name OR email matches `assign`. Case-insensitive on both fields. @@ -161,31 +271,14 @@ func (d *HTTPHandlerDispatcher) dispatchItemUpdate( itemPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/items/" + url.PathEscape(ref) - // `--role` parity with mapItemCreate: until the next route-table - // expansion adds slug → ID resolution, reject loudly so agents - // don't get a successful update response while their requested - // role assignment is silently dropped (Codex review #345 round 1). - // The workaround pointed at — `--field agent_role_id=` — - // is the genuinely-reachable path: --field IS in the auto- - // generated tool schema, and the dispatcher's - // liftFieldsToColumns helper recognizes column keys and moves - // them out of the fields JSON onto the top-level payload before - // PATCHing. (Codex review #345 rounds 2+3 walked through both - // the original "agent_role_id at top level" suggestion that - // agents couldn't reach via the schema, and the original - // "--field agent_role_id" suggestion that would have stuffed - // the value inert into the fields blob.) - if v, ok := input["role"].(string); ok && v != "" { - return mcp.NewToolResultErrorf( - "%s: --role is not yet supported by HTTPHandlerDispatcher; "+ - "slug → role-ID resolution lands in the next route-table "+ - "expansion. For now, pass `--field agent_role_id=` — "+ - "the dispatcher recognizes column keys from --field and "+ - "writes them to the column rather than the fields blob. "+ - "Use `role list` to find the UUID.", - cmdKey, - ), nil - } + // `--role` is now resolved at the dispatcher level (TASK-968): + // Dispatch's preprocess step rewrites it to `agent_role_id` + // before reaching this method, so by the time we get here a slug + // has already been resolved to a UUID. The `--field + // agent_role_id=` workaround that the older rejection + // pointed at still works (lifted via liftFieldsToColumns below) + // and is preserved as the explicit-ID escape hatch when an agent + // already knows the UUID and wants to skip the slug lookup. // Step 1: GET the existing item so we can read fields for the // read-modify-write merge. If the GET fails (item not found, diff --git a/internal/mcp/dispatch_http_advanced_test.go b/internal/mcp/dispatch_http_advanced_test.go index 67902547..b777722a 100644 --- a/internal/mcp/dispatch_http_advanced_test.go +++ b/internal/mcp/dispatch_http_advanced_test.go @@ -570,34 +570,92 @@ func TestDispatchItemUpdate_PassesThroughAgentRoleID(t *testing.T) { } } -func TestDispatchItemUpdate_RejectsUnsupportedRole(t *testing.T) { - // Parity with mapItemCreate's role rejection — until the next - // route-table expansion adds slug → ID resolution, agents must - // not get a "success" response while their role assignment was - // silently dropped (Codex review #345 round 1). - prefetchCount := 0 +func TestDispatchItemUpdate_ResolvesRoleSlugToAgentRoleID(t *testing.T) { + // TASK-968 replaced the older --role rejection with a Dispatch- + // level preprocess that resolves slug → agent_role_id via the + // /agent-roles/{slug} endpoint. The PATCH body should carry the + // resolved UUID in `agent_role_id` and not in `fields`, AND the + // `role` key should be gone from the input by the time the + // handler sees it. + captured := newRequestCapture() mux := http.NewServeMux() - mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(_ http.ResponseWriter, _ *http.Request) { - prefetchCount++ + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/implementer", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"role-uuid-imp","slug":"implementer","name":"Implementer"}`)) }) - d := &HTTPHandlerDispatcher{ - Handler: mux, - UserResolver: fixedUserResolver(&models.User{ID: "caller"}), + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5","fields":"{\"status\":\"open\"}"}`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + default: + t.Fatalf("unexpected method %s", r.Method) + } + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + "role": "implementer", + }) + res, err := d.Dispatch(ctx, []string{"item", "update"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["agent_role_id"] != "role-uuid-imp" { + t.Errorf("expected agent_role_id from slug resolution; got %v", body) + } + // And NOT inside fields blob. + fields := map[string]any{} + if s, _ := body["fields"].(string); s != "" { + _ = json.Unmarshal([]byte(s), &fields) + } + if _, present := fields["agent_role_id"]; present { + t.Errorf("agent_role_id should not be in fields blob: %v", fields) + } + if _, present := body["role"]; present { + t.Errorf("`role` key should be removed after resolution: %v", body) + } +} + +func TestDispatchItemUpdate_RoleResolutionFailureSurfacesAsToolError(t *testing.T) { + // Slug-not-found at the agent-roles endpoint must abort the + // dispatch — the older rejection guarded against silent drops + // of the role assignment, and the new preprocess preserves that + // guarantee on a different code path. + patchCount := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/ghost", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"not found"}`)) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(_ http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch { + patchCount++ + } + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} ctx := WithDispatchInput(context.Background(), map[string]any{ "workspace": "docapp", "ref": "TASK-5", - "role": "implementer", + "role": "ghost", }) res, err := d.Dispatch(ctx, []string{"item", "update"}, nil) if err != nil { t.Fatalf("Dispatch err: %v", err) } if !res.IsError { - t.Errorf("expected IsError when --role passed; got %#v", res) + t.Errorf("expected IsError when role resolution fails; got %#v", res) } - // And: no handler call AT ALL — neither prefetch nor PATCH. - if prefetchCount != 0 { - t.Errorf("handler must not run when --role rejection trips; ran %d times", prefetchCount) + if patchCount != 0 { + t.Errorf("PATCH must not run after role resolution failure; ran %d times", patchCount) } } diff --git a/internal/mcp/dispatch_http_links.go b/internal/mcp/dispatch_http_links.go new file mode 100644 index 00000000..4b3f913b --- /dev/null +++ b/internal/mcp/dispatch_http_links.go @@ -0,0 +1,672 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + + "github.com/mark3labs/mcp-go/mcp" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// itemLinkSpec wires a CLI link-command's arg shape to the underlying +// /api/v1/workspaces/{ws}/items/{slug}/links surface. +// +// The link-command surface has an asymmetry the route table can't +// express: the URL path goes through ONE item's slug while the body +// carries the OTHER item's UUID. Block/implements/supersedes/split-from +// use (source.Slug, target.ID); blocked-by inverts to (blocker.Slug, +// source.ID). The mapper would need to know which input key drives +// which side, and would still need a Handler reference to do the +// ref→ID prefetch for the body's target_id (the body shape rejects +// a raw ref). +// +// Lives as method-bound dispatchers on HTTPHandlerDispatcher rather +// than mappers in the route table for the same reason dispatchItemUpdate +// does — needs Handler to do the ref→{slug,id} prefetches before +// building the request. +type itemLinkSpec struct { + // cmdKey is the dotted command path, e.g. "item block" — used as + // the prefix on every error / IsError result so MCP clients see a + // stable identifier. + cmdKey string + + // urlRefKey is the input key whose resolved item.Slug goes into + // the URL path /items/{slug}/links. For `item block` that's + // `source_ref`; for `item blocked-by` it's `blocker_ref` (the + // blocker is the link's source per the data model). + urlRefKey string + + // bodyTargetRefKey is the input key whose resolved item.ID + // becomes the body's target_id. For `item block` that's + // `target_ref`; for `item blocked-by` it's `source_ref` (the + // blocked item is the link's target). + bodyTargetRefKey string + + // linkType is the canonical type written on the wire — must be + // one of the constants in models/item_links.go (blocks, + // implements, supersedes, split_from). + linkType string +} + +// itemLinkSpecs is the lookup table for link create/delete commands. +// Build is in init() so the package's startup-cost stays small and +// the cmdKey constants are co-located with their wiring. +// +// Read-only link commands (deps, related, implemented-by) are not +// here — those just GET /items/{ref}/links and don't need the URL/body +// asymmetry; they go through dispatchGetItemLinks instead. +var itemLinkSpecs = map[string]itemLinkSpec{ + // `block`: SOURCE blocks TARGET. Link source = source_ref item, + // link target = target_ref item. + "item block": { + cmdKey: "item block", + urlRefKey: "source_ref", + bodyTargetRefKey: "target_ref", + linkType: models.ItemLinkTypeBlocks, + }, + // `blocked-by`: SOURCE is blocked by BLOCKER → blocker blocks + // source. The link's source is BLOCKER, target is SOURCE. + "item blocked-by": { + cmdKey: "item blocked-by", + urlRefKey: "blocker_ref", + bodyTargetRefKey: "source_ref", + linkType: models.ItemLinkTypeBlocks, + }, + "item unblock": { + cmdKey: "item unblock", + urlRefKey: "source_ref", + bodyTargetRefKey: "target_ref", + linkType: models.ItemLinkTypeBlocks, + }, + "item implements": { + cmdKey: "item implements", + urlRefKey: "implementer_ref", + bodyTargetRefKey: "target_ref", + linkType: models.ItemLinkTypeImplements, + }, + "item unimplements": { + cmdKey: "item unimplements", + urlRefKey: "implementer_ref", + bodyTargetRefKey: "target_ref", + linkType: models.ItemLinkTypeImplements, + }, + "item supersedes": { + cmdKey: "item supersedes", + urlRefKey: "new_ref", + bodyTargetRefKey: "old_ref", + linkType: models.ItemLinkTypeSupersedes, + }, + "item unsupersede": { + cmdKey: "item unsupersede", + urlRefKey: "new_ref", + bodyTargetRefKey: "old_ref", + linkType: models.ItemLinkTypeSupersedes, + }, + "item split-from": { + cmdKey: "item split-from", + urlRefKey: "child_ref", + bodyTargetRefKey: "parent_ref", + linkType: models.ItemLinkTypeSplitFrom, + }, + "item unsplit": { + cmdKey: "item unsplit", + urlRefKey: "child_ref", + bodyTargetRefKey: "parent_ref", + linkType: models.ItemLinkTypeSplitFrom, + }, +} + +// itemPrefetch is the shape resolveItemRef returns. Only exposes the +// fields the link dispatchers need so callers can't accidentally lean +// on something that's only sometimes populated. +type itemPrefetch struct { + ID string `json:"id"` + Slug string `json:"slug"` +} + +// resolveItemRef does a GET /api/v1/workspaces/{ws}/items/{ref} and +// returns the resolved id+slug. Used by the link dispatchers to +// translate user-friendly refs (TASK-5, item slugs, UUIDs) into the +// {slug for URL, id for body} pair the /links surface expects. +// +// Goes through buildAuthedRequest so any OAuth-scope context attached +// at dispatch time (d.Apply) applies to the prefetch the same way it +// applies to the main request — same scope-bypass-prevention reasoning +// behind dispatchItemUpdate's prefetch (Codex review #345 round 1). +func (d *HTTPHandlerDispatcher) resolveItemRef( + ctx context.Context, + user *models.User, + workspace, ref string, +) (*itemPrefetch, error) { + path := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/items/" + url.PathEscape(ref) + req, err := d.buildAuthedRequest(ctx, http.MethodGet, path, nil, user) + if err != nil { + return nil, fmt.Errorf("build prefetch request for %q: %w", ref, err) + } + rec := httptest.NewRecorder() + d.Handler.ServeHTTP(rec, req) + if rec.Code >= 400 { + body := strings.TrimSpace(rec.Body.String()) + if body == "" { + body = http.StatusText(rec.Code) + } + return nil, fmt.Errorf("resolve %q: %d %s", ref, rec.Code, body) + } + var out itemPrefetch + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + return nil, fmt.Errorf("parse item prefetch for %q: %w", ref, err) + } + if out.ID == "" || out.Slug == "" { + return nil, fmt.Errorf("resolve %q: response missing id or slug", ref) + } + return &out, nil +} + +// dispatchCreateItemLink handles the create-side of the link surface +// (block, blocked-by, implements, supersedes, split-from). Resolves +// both refs, then POSTs to /items/{urlSlug}/links with body +// {target_id: , link_type: }. +// +// Mirrors the CLI's createLineageLink (cmd/pad/lineage.go) and +// blocksCmd / blockedByCmd (cmd/pad/main.go). The behaviour is +// identical: prefetch source + target, then create the link in the +// canonical direction the data model expects. +func (d *HTTPHandlerDispatcher) dispatchCreateItemLink( + ctx context.Context, + input map[string]any, + user *models.User, + spec itemLinkSpec, +) (*mcp.CallToolResult, error) { + workspace, _ := input["workspace"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", spec.cmdKey), nil + } + + urlRef, _ := input[spec.urlRefKey].(string) + if urlRef == "" { + return mcp.NewToolResultErrorf("%s: %s is required", spec.cmdKey, spec.urlRefKey), nil + } + bodyRef, _ := input[spec.bodyTargetRefKey].(string) + if bodyRef == "" { + return mcp.NewToolResultErrorf("%s: %s is required", spec.cmdKey, spec.bodyTargetRefKey), nil + } + + urlItem, err := d.resolveItemRef(ctx, user, workspace, urlRef) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", spec.cmdKey, err.Error()), nil + } + bodyItem, err := d.resolveItemRef(ctx, user, workspace, bodyRef) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", spec.cmdKey, err.Error()), nil + } + + payload := map[string]any{ + "target_id": bodyItem.ID, + "link_type": spec.linkType, + } + body, err := json.Marshal(payload) + if err != nil { + return mcp.NewToolResultErrorf("%s: encode body: %s", spec.cmdKey, err.Error()), nil + } + + urlPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/items/" + url.PathEscape(urlItem.Slug) + "/links" + return d.executeRequest(ctx, spec.cmdKey, user, http.MethodPost, urlPath, body) +} + +// dispatchDeleteItemLink handles the un-* side of the link surface +// (unblock, unimplements, unsupersede, unsplit). Mirrors the CLI's +// deleteLineageLink + unblockCmd: resolve both refs, list links on +// the source item, find the one matching (source.ID, target.ID, +// link_type), and DELETE it by id. +// +// Returns IsError when no matching link exists — same UX the CLI +// surfaces ("no relationship found"). Surfacing the same +// missing-link error keeps the behaviour identical across transports. +func (d *HTTPHandlerDispatcher) dispatchDeleteItemLink( + ctx context.Context, + input map[string]any, + user *models.User, + spec itemLinkSpec, +) (*mcp.CallToolResult, error) { + workspace, _ := input["workspace"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", spec.cmdKey), nil + } + + urlRef, _ := input[spec.urlRefKey].(string) + if urlRef == "" { + return mcp.NewToolResultErrorf("%s: %s is required", spec.cmdKey, spec.urlRefKey), nil + } + bodyRef, _ := input[spec.bodyTargetRefKey].(string) + if bodyRef == "" { + return mcp.NewToolResultErrorf("%s: %s is required", spec.cmdKey, spec.bodyTargetRefKey), nil + } + + urlItem, err := d.resolveItemRef(ctx, user, workspace, urlRef) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", spec.cmdKey, err.Error()), nil + } + bodyItem, err := d.resolveItemRef(ctx, user, workspace, bodyRef) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", spec.cmdKey, err.Error()), nil + } + + links, err := d.listItemLinks(ctx, user, workspace, urlItem.Slug) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", spec.cmdKey, err.Error()), nil + } + + canonicalType, normErr := models.NormalizeItemLinkType(spec.linkType) + if normErr != nil { + // Programming error — only canonical types belong in itemLinkSpecs. + return mcp.NewToolResultErrorf("%s: invalid link type %q", spec.cmdKey, spec.linkType), nil + } + + var linkID string + for _, link := range links { + if link.SourceID != urlItem.ID || link.TargetID != bodyItem.ID { + continue + } + got, err := models.NormalizeItemLinkType(link.LinkType) + if err != nil { + continue + } + if got == canonicalType { + linkID = link.ID + break + } + } + if linkID == "" { + return mcp.NewToolResultErrorf( + "%s: no %s relationship found between %s and %s", + spec.cmdKey, spec.linkType, urlRef, bodyRef, + ), nil + } + + urlPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/links/" + url.PathEscape(linkID) + res, err := d.executeRequest(ctx, spec.cmdKey, user, http.MethodDelete, urlPath, nil) + if err != nil || res.IsError { + return res, err + } + // Handler returns 204 No Content — packageHTTPResponse turns an + // empty body into an empty TextContent, which is uninformative + // for MCP clients. Mirror the CLI's `--format json` output for + // these commands (cmd/pad/main.go's unblockCmd / lineage + // deleteLineageLink: `{"status":"removed"}`) so MCP gets a + // structured success signal. Goes through packageStructuredResponse + // so the StructuredContent is a JSON-decoded `map[string]any`, + // matching what the rest of the dispatcher emits. + return packageStructuredResponse(spec.cmdKey, map[string]string{"status": "removed"}) +} + +// dispatchItemDeps handles `pad item deps ` — the simplest of +// the three read-only link queries. CLI parity: `deps --format json` +// returns the raw `/links` array, so we just GET-and-package. +func (d *HTTPHandlerDispatcher) dispatchItemDeps( + ctx context.Context, + input map[string]any, + user *models.User, +) (*mcp.CallToolResult, error) { + const cmdKey = "item deps" + workspace, _ := input["workspace"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", cmdKey), nil + } + ref, _ := input["ref"].(string) + if ref == "" { + return mcp.NewToolResultErrorf("%s: ref is required", cmdKey), nil + } + + urlPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/items/" + url.PathEscape(ref) + "/links" + return d.executeRequest(ctx, cmdKey, user, http.MethodGet, urlPath, nil) +} + +// relatedEntry / relatedGroup mirror the CLI's `--format json` output +// for `item related` / `item implemented-by` (cmd/pad/query.go's +// types of the same name). Reproduced here because the CLI types are +// in package main and not importable; field shapes match exactly so +// MCP clients see the same JSON they'd see through the CLI. +type relatedEntry struct { + Ref string `json:"ref,omitempty"` + Title string `json:"title"` + CollectionSlug string `json:"collection_slug,omitempty"` + Status string `json:"status,omitempty"` +} + +type relatedGroup struct { + Key string `json:"key"` + Label string `json:"label"` + Entries []relatedEntry `json:"entries"` +} + +// dispatchItemRelated handles `pad item related ` and emits the +// grouped response shape the CLI's `--format json` output uses +// (cmd/pad/query.go relatedCmd): +// +// {"item_ref":..., "item_title":..., "collection":..., +// "group_count": N, "groups": [{"key", "label", "entries":[...]}, ...]} +// +// Codex review on PR #346 caught the original raw-links shape as a +// behavioural divergence from the CLI; fixing it preserves the +// "transport-equivalent to ExecDispatcher" contract this dispatcher +// is built on. +func (d *HTTPHandlerDispatcher) dispatchItemRelated( + ctx context.Context, + input map[string]any, + user *models.User, +) (*mcp.CallToolResult, error) { + const cmdKey = "item related" + workspace, _ := input["workspace"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", cmdKey), nil + } + ref, _ := input["ref"].(string) + if ref == "" { + return mcp.NewToolResultErrorf("%s: ref is required", cmdKey), nil + } + + item, err := d.fetchItem(ctx, user, workspace, ref) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", cmdKey, err.Error()), nil + } + links, err := d.listItemLinks(ctx, user, workspace, item.Slug) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", cmdKey, err.Error()), nil + } + + groups := buildRelatedGroups(item, links) + payload := map[string]any{ + "item_ref": itemRefString(item), + "item_title": item.Title, + "collection": item.CollectionSlug, + "group_count": len(groups), + "groups": groups, + } + return packageStructuredResponse(cmdKey, payload) +} + +// dispatchItemImplementedBy handles `pad item implemented-by ` +// with the CLI's filtered shape (cmd/pad/query.go implementedByCmd): +// +// {"item_ref":..., "item_title":..., "count": N, +// "results": [{"ref","title","collection_slug","status"}, ...]} +// +// Filters to INCOMING `implements` links only — outgoing implements +// links are excluded because they describe what THIS item implements, +// not what implements it. +func (d *HTTPHandlerDispatcher) dispatchItemImplementedBy( + ctx context.Context, + input map[string]any, + user *models.User, +) (*mcp.CallToolResult, error) { + const cmdKey = "item implemented-by" + workspace, _ := input["workspace"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", cmdKey), nil + } + ref, _ := input["ref"].(string) + if ref == "" { + return mcp.NewToolResultErrorf("%s: ref is required", cmdKey), nil + } + + item, err := d.fetchItem(ctx, user, workspace, ref) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", cmdKey, err.Error()), nil + } + links, err := d.listItemLinks(ctx, user, workspace, item.Slug) + if err != nil { + return mcp.NewToolResultErrorf("%s: %s", cmdKey, err.Error()), nil + } + + results := incomingImplementedBy(item, links) + payload := map[string]any{ + "item_ref": itemRefString(item), + "item_title": item.Title, + "count": len(results), + "results": results, + } + return packageStructuredResponse(cmdKey, payload) +} + +// packageStructuredResponse encodes payload to JSON, then decodes it +// back to a generic any so the StructuredContent surface matches the +// shape MCP clients see over the wire — `map[string]any` / `[]any` / +// JSON-decoded primitives — rather than the originally-typed Go +// struct slices. +// +// This matches packageHTTPResponse's pattern: that helper json-decodes +// the handler's response body into `any` for the structured channel, +// so synthesized responses use the same path here for shape parity. +func packageStructuredResponse(cmdKey string, payload any) (*mcp.CallToolResult, error) { + body, err := json.Marshal(payload) + if err != nil { + return mcp.NewToolResultErrorf("%s: encode response: %s", cmdKey, err.Error()), nil + } + var decoded any + if err := json.Unmarshal(body, &decoded); err != nil { + // Should be unreachable — we just marshalled a known-good + // payload — but if it ever happens, fall back to the typed + // payload + raw body so the caller still gets something + // usable instead of an error. + return mcp.NewToolResultStructured(payload, string(body)), nil + } + return mcp.NewToolResultStructured(decoded, string(body)), nil +} + +// fetchItem retrieves a full models.Item via the +// /api/v1/workspaces/{ws}/items/{ref} surface. Used by the related / +// implemented-by dispatchers which need title + collection_slug + +// computed ref for the response wrapper. Goes through buildAuthedRequest +// so d.Apply (OAuth scope context) applies to the prefetch — same +// scope-bypass-prevention reasoning behind dispatchItemUpdate's GET. +func (d *HTTPHandlerDispatcher) fetchItem( + ctx context.Context, + user *models.User, + workspace, ref string, +) (*models.Item, error) { + path := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/items/" + url.PathEscape(ref) + req, err := d.buildAuthedRequest(ctx, http.MethodGet, path, nil, user) + if err != nil { + return nil, fmt.Errorf("build item request for %q: %w", ref, err) + } + rec := httptest.NewRecorder() + d.Handler.ServeHTTP(rec, req) + if rec.Code >= 400 { + body := strings.TrimSpace(rec.Body.String()) + if body == "" { + body = http.StatusText(rec.Code) + } + return nil, fmt.Errorf("resolve %q: %d %s", ref, rec.Code, body) + } + var item models.Item + if err := json.Unmarshal(rec.Body.Bytes(), &item); err != nil { + return nil, fmt.Errorf("parse item response for %q: %w", ref, err) + } + return &item, nil +} + +// itemRefString returns "TASK-5"-style refs for non-nil items, or +// the empty string when the collection prefix or item number is +// missing. Mirrors cli.ItemRef but operates on a pointer so the +// dispatchers can pass *models.Item without dereferencing. +func itemRefString(item *models.Item) string { + if item == nil || item.CollectionPrefix == "" || item.ItemNumber == nil { + return "" + } + return fmt.Sprintf("%s-%d", item.CollectionPrefix, *item.ItemNumber) +} + +// buildRelatedGroups mirrors cmd/pad/query.go's function of the same +// name. Reproduced here because the CLI version is in package main. +// Groups every link touching `item` by canonical type + direction +// (split_from vs split_into, supersedes vs superseded_by, etc.) and +// returns a stable-ordered list. +func buildRelatedGroups(item *models.Item, links []models.ItemLink) []relatedGroup { + if item == nil || len(links) == 0 { + return []relatedGroup{} + } + + type groupDef struct{ label string } + definitions := map[string]groupDef{ + "blocks": {label: "Blocks"}, + "blocked_by": {label: "Blocked by"}, + "links_to": {label: "Links to"}, + "referenced_by": {label: "Referenced by"}, + "split_from": {label: "Split from"}, + "split_into": {label: "Split into"}, + "supersedes": {label: "Supersedes"}, + "superseded_by": {label: "Superseded by"}, + "implements": {label: "Implements"}, + "implemented_by": {label: "Implemented by"}, + "related": {label: "Related"}, + } + order := []string{ + "blocks", "blocked_by", + "links_to", "referenced_by", + "split_from", "split_into", + "supersedes", "superseded_by", + "implements", "implemented_by", + "related", + } + + grouped := map[string][]relatedEntry{} + for _, link := range links { + linkType, err := models.NormalizeItemLinkType(link.LinkType) + if err != nil { + linkType = models.ItemLinkTypeRelated + } + isSource := link.SourceID == item.ID + + switch linkType { + case models.ItemLinkTypeBlocks: + if isSource { + grouped["blocks"] = append(grouped["blocks"], relatedEntryFromLink(link, false)) + } else { + grouped["blocked_by"] = append(grouped["blocked_by"], relatedEntryFromLink(link, true)) + } + case models.ItemLinkTypeWikiLink: + if isSource { + grouped["links_to"] = append(grouped["links_to"], relatedEntryFromLink(link, false)) + } else { + grouped["referenced_by"] = append(grouped["referenced_by"], relatedEntryFromLink(link, true)) + } + case models.ItemLinkTypeSplitFrom: + if isSource { + grouped["split_from"] = append(grouped["split_from"], relatedEntryFromLink(link, false)) + } else { + grouped["split_into"] = append(grouped["split_into"], relatedEntryFromLink(link, true)) + } + case models.ItemLinkTypeSupersedes: + if isSource { + grouped["supersedes"] = append(grouped["supersedes"], relatedEntryFromLink(link, false)) + } else { + grouped["superseded_by"] = append(grouped["superseded_by"], relatedEntryFromLink(link, true)) + } + case models.ItemLinkTypeImplements: + if isSource { + grouped["implements"] = append(grouped["implements"], relatedEntryFromLink(link, false)) + } else { + grouped["implemented_by"] = append(grouped["implemented_by"], relatedEntryFromLink(link, true)) + } + default: + grouped["related"] = append(grouped["related"], relatedEntryFromLink(link, !isSource)) + } + } + + results := make([]relatedGroup, 0, len(order)) + for _, key := range order { + entries := grouped[key] + if len(entries) == 0 { + continue + } + results = append(results, relatedGroup{ + Key: key, + Label: definitions[key].label, + Entries: entries, + }) + } + return results +} + +// incomingImplementedBy mirrors cmd/pad/query.go's helper. Filters +// the link list to incoming `implements` links only — outgoing +// implements describe what THIS item implements, which is the +// reverse of what callers want. +func incomingImplementedBy(item *models.Item, links []models.ItemLink) []relatedEntry { + if item == nil { + return []relatedEntry{} + } + results := make([]relatedEntry, 0, len(links)) + for _, link := range links { + linkType, err := models.NormalizeItemLinkType(link.LinkType) + if err != nil { + continue + } + if linkType != models.ItemLinkTypeImplements || link.TargetID != item.ID { + continue + } + results = append(results, relatedEntryFromLink(link, true)) + } + return results +} + +// relatedEntryFromLink projects a link's source-side or target-side +// metadata into a relatedEntry. Mirrors cmd/pad/query.go's helper. +func relatedEntryFromLink(link models.ItemLink, useSource bool) relatedEntry { + if useSource { + return relatedEntry{ + Ref: link.SourceRef, + Title: link.SourceTitle, + CollectionSlug: link.SourceCollectionSlug, + Status: link.SourceStatus, + } + } + return relatedEntry{ + Ref: link.TargetRef, + Title: link.TargetTitle, + CollectionSlug: link.TargetCollectionSlug, + Status: link.TargetStatus, + } +} + +// listItemLinks issues an in-handler GET against +// /api/v1/workspaces/{ws}/items/{slug}/links and decodes the response +// into models.ItemLink so the un-* dispatchers can find the matching +// link to delete. +func (d *HTTPHandlerDispatcher) listItemLinks( + ctx context.Context, + user *models.User, + workspace, itemSlug string, +) ([]models.ItemLink, error) { + path := "/api/v1/workspaces/" + url.PathEscape(workspace) + + "/items/" + url.PathEscape(itemSlug) + "/links" + req, err := d.buildAuthedRequest(ctx, http.MethodGet, path, nil, user) + if err != nil { + return nil, fmt.Errorf("build links request: %w", err) + } + rec := httptest.NewRecorder() + d.Handler.ServeHTTP(rec, req) + if rec.Code >= 400 { + body := strings.TrimSpace(rec.Body.String()) + if body == "" { + body = http.StatusText(rec.Code) + } + return nil, fmt.Errorf("list item links: %d %s", rec.Code, body) + } + var links []models.ItemLink + if err := json.Unmarshal(rec.Body.Bytes(), &links); err != nil { + return nil, fmt.Errorf("parse links response: %w", err) + } + return links, nil +} diff --git a/internal/mcp/dispatch_http_links_test.go b/internal/mcp/dispatch_http_links_test.go new file mode 100644 index 00000000..129718b0 --- /dev/null +++ b/internal/mcp/dispatch_http_links_test.go @@ -0,0 +1,934 @@ +package mcp + +import ( + "context" + "encoding/json" + "io" + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// itemPrefetchHandler routes /api/v1/workspaces/{ws}/items/{ref} GETs +// to a static map keyed by ref so each link-test can declare just the +// items it needs. Other paths fall through to next so the same mux +// can host the actual link endpoint. +func itemPrefetchHandler(t *testing.T, items map[string]itemPrefetch) http.HandlerFunc { + t.Helper() + return func(w http.ResponseWriter, r *http.Request) { + // Only handle the GET-on-item shape; let the caller decide + // what to do with everything else. + if r.Method != http.MethodGet { + http.NotFound(w, r) + return + } + // Path is /api/v1/workspaces/{ws}/items/{ref}; pull the last + // segment as the ref. + parts := strings.Split(strings.TrimPrefix(r.URL.Path, "/"), "/") + if len(parts) < 6 { + http.NotFound(w, r) + return + } + ref := parts[5] + // Subpaths like .../items/{ref}/links shouldn't hit this — the + // path will have more than 6 segments. Only match the leaf + // item route. + if len(parts) != 6 { + http.NotFound(w, r) + return + } + got, ok := items[ref] + if !ok { + http.Error(w, `{"error":"item not found"}`, http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(got) + } +} + +// linkCaptureHandler records every POST/DELETE on the link endpoint +// under /items/{ref}/links or /links/{linkID} so the create/delete +// tests can assert on path + body. Returns 201/204 with a small JSON +// body so packageHTTPResponse's success branch is exercised. +type linkCaptureHandler struct { + t *testing.T + postCount int + deleteCount int + lastPostPath string + lastPostBody string + lastDeletePath string + respMembers []models.ItemLink +} + +func (h *linkCaptureHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + h.postCount++ + h.lastPostPath = r.URL.Path + buf, _ := io.ReadAll(r.Body) + h.lastPostBody = string(buf) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"new-link","link_type":"blocks"}`)) + case http.MethodDelete: + h.deleteCount++ + h.lastDeletePath = r.URL.Path + w.WriteHeader(http.StatusNoContent) + case http.MethodGet: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(h.respMembers) + default: + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + } +} + +// --- Create-link dispatchers --- + +func TestDispatch_ItemBlock_ResolvesAndPostsLink(t *testing.T) { + items := map[string]itemPrefetch{ + "TASK-5": {ID: "id-task-5", Slug: "task-5"}, + "TASK-8": {ID: "id-task-8", Slug: "task-8"}, + } + cap := &linkCaptureHandler{t: t} + mux := http.NewServeMux() + mux.Handle("/api/v1/workspaces/docapp/items/task-5/links", cap) + // Item-resolution catches every other GET on /items/{ref}. + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "source_ref": "TASK-5", + "target_ref": "TASK-8", + }) + res, err := d.Dispatch(ctx, []string{"item", "block"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if cap.postCount != 1 { + t.Fatalf("expected 1 POST, got %d", cap.postCount) + } + if cap.lastPostPath != "/api/v1/workspaces/docapp/items/task-5/links" { + t.Errorf("URL slug should come from source_ref; got %q", cap.lastPostPath) + } + var body map[string]any + if err := json.Unmarshal([]byte(cap.lastPostBody), &body); err != nil { + t.Fatalf("decode body: %v\n%s", err, cap.lastPostBody) + } + if body["target_id"] != "id-task-8" { + t.Errorf("target_id should be target_ref's resolved ID; got %v", body) + } + if body["link_type"] != "blocks" { + t.Errorf("link_type = %v, want blocks", body["link_type"]) + } +} + +func TestDispatch_ItemBlockedBy_InvertsURLAndBody(t *testing.T) { + // `blocked-by` writes the link as (blocker → source). The URL + // path goes through the BLOCKER's slug, body's target_id is the + // SOURCE item's id. This is the asymmetry that makes the + // link-spec table necessary. + items := map[string]itemPrefetch{ + "TASK-5": {ID: "id-task-5", Slug: "task-5"}, // blocked + "TASK-3": {ID: "id-task-3", Slug: "task-3"}, // blocker + } + cap := &linkCaptureHandler{t: t} + mux := http.NewServeMux() + mux.Handle("/api/v1/workspaces/docapp/items/task-3/links", cap) // blocker's URL + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "source_ref": "TASK-5", + "blocker_ref": "TASK-3", + }) + res, err := d.Dispatch(ctx, []string{"item", "blocked-by"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if cap.postCount != 1 { + t.Fatalf("expected 1 POST, got %d", cap.postCount) + } + if cap.lastPostPath != "/api/v1/workspaces/docapp/items/task-3/links" { + t.Errorf("URL slug must come from blocker_ref (task-3); got %q", cap.lastPostPath) + } + var body map[string]any + if err := json.Unmarshal([]byte(cap.lastPostBody), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["target_id"] != "id-task-5" { + t.Errorf("target_id must be source_ref's resolved ID (id-task-5); got %v", body) + } +} + +func TestDispatch_ItemImplements_PostsLineageLink(t *testing.T) { + items := map[string]itemPrefetch{ + "TASK-9": {ID: "id-task-9", Slug: "task-9"}, + "IDEA-12": {ID: "id-idea-12", Slug: "idea-12"}, + } + cap := &linkCaptureHandler{t: t} + mux := http.NewServeMux() + mux.Handle("/api/v1/workspaces/docapp/items/task-9/links", cap) + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "implementer_ref": "TASK-9", + "target_ref": "IDEA-12", + }) + res, err := d.Dispatch(ctx, []string{"item", "implements"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if cap.lastPostPath != "/api/v1/workspaces/docapp/items/task-9/links" { + t.Errorf("URL slug = %q", cap.lastPostPath) + } + var body map[string]any + if err := json.Unmarshal([]byte(cap.lastPostBody), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["link_type"] != models.ItemLinkTypeImplements { + t.Errorf("link_type = %v", body["link_type"]) + } + if body["target_id"] != "id-idea-12" { + t.Errorf("target_id = %v", body["target_id"]) + } +} + +func TestDispatch_ItemSplitFrom_UsesCanonicalLinkType(t *testing.T) { + // CLI accepts "split-from" but the canonical wire form is + // "split_from" (models.ItemLinkTypeSplitFrom). Make sure the + // dispatcher writes the canonical form to the body. + items := map[string]itemPrefetch{ + "TASK-22": {ID: "id-22", Slug: "task-22"}, + "TASK-21": {ID: "id-21", Slug: "task-21"}, + } + cap := &linkCaptureHandler{t: t} + mux := http.NewServeMux() + mux.Handle("/api/v1/workspaces/docapp/items/task-22/links", cap) + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "child_ref": "TASK-22", + "parent_ref": "TASK-21", + }) + res, err := d.Dispatch(ctx, []string{"item", "split-from"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + var body map[string]any + if err := json.Unmarshal([]byte(cap.lastPostBody), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body["link_type"] != "split_from" { + t.Errorf("link_type should be canonical \"split_from\"; got %v", body["link_type"]) + } +} + +func TestDispatch_ItemBlock_MissingRefSurfacesAsToolError(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not call handler when refs missing"), + UserResolver: fixedUserResolver(&models.User{ID: "u"}), + } + for _, missing := range []string{"workspace", "source_ref", "target_ref"} { + t.Run("missing-"+missing, func(t *testing.T) { + input := map[string]any{ + "workspace": "ws", + "source_ref": "A", + "target_ref": "B", + } + delete(input, missing) + ctx := WithDispatchInput(context.Background(), input) + res, err := d.Dispatch(ctx, []string{"item", "block"}, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError when %s missing", missing) + } + }) + } +} + +func TestDispatch_ItemBlock_PrefetchFailureBlocksPost(t *testing.T) { + // If the source ref doesn't resolve, the link POST must not run. + postCount := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + postCount++ + return + } + w.WriteHeader(http.StatusNotFound) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "source_ref": "TASK-5", "target_ref": "TASK-8", + }) + res, err := d.Dispatch(ctx, []string{"item", "block"}, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError after prefetch 404; got %#v", res) + } + if postCount != 0 { + t.Errorf("link POST must not run after prefetch failure; ran %d times", postCount) + } +} + +// --- Delete-link dispatchers --- + +func TestDispatch_ItemUnblock_FindsAndDeletesMatchingLink(t *testing.T) { + items := map[string]itemPrefetch{ + "TASK-5": {ID: "id-5", Slug: "task-5"}, + "TASK-8": {ID: "id-8", Slug: "task-8"}, + } + mux := http.NewServeMux() + + // Links list endpoint returns one matching `blocks` link plus + // some noise so the delete dispatcher has to actually find the + // right ID. + mux.HandleFunc("/api/v1/workspaces/docapp/items/task-5/links", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected", http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]models.ItemLink{ + {ID: "wrong-link-1", SourceID: "id-5", TargetID: "id-8", LinkType: "implements"}, // wrong type + {ID: "wrong-link-2", SourceID: "id-5", TargetID: "id-99", LinkType: "blocks"}, // wrong target + {ID: "the-link", SourceID: "id-5", TargetID: "id-8", LinkType: "blocks"}, + }) + }) + + deleteCount := 0 + deletedID := "" + mux.HandleFunc("/api/v1/workspaces/docapp/links/", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + http.Error(w, "unexpected", http.StatusMethodNotAllowed) + return + } + deleteCount++ + deletedID = strings.TrimPrefix(r.URL.Path, "/api/v1/workspaces/docapp/links/") + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "source_ref": "TASK-5", "target_ref": "TASK-8", + }) + res, err := d.Dispatch(ctx, []string{"item", "unblock"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if deleteCount != 1 { + t.Fatalf("expected 1 DELETE, got %d", deleteCount) + } + if deletedID != "the-link" { + t.Errorf("deleted wrong link; got %q want %q", deletedID, "the-link") + } +} + +func TestDispatch_ItemUnblock_WrapsDelete204AsStatusRemoved(t *testing.T) { + // Handler returns 204 No Content on DELETE. The CLI's + // `--format json` for `unblock` (and the lineage delete-link + // commands) prints `{"status":"removed"}`. The dispatcher must + // match — Codex review caught the empty-text packaging as a + // divergence. + items := map[string]itemPrefetch{ + "TASK-5": {ID: "id-5", Slug: "task-5"}, + "TASK-8": {ID: "id-8", Slug: "task-8"}, + } + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/task-5/links", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]models.ItemLink{ + {ID: "the-link", SourceID: "id-5", TargetID: "id-8", LinkType: "blocks"}, + }) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/links/", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "source_ref": "TASK-5", "target_ref": "TASK-8", + }) + res, err := d.Dispatch(ctx, []string{"item", "unblock"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + payload, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected structured map[string]any (JSON-decoded shape); got %#v", res.StructuredContent) + } + if payload["status"] != "removed" { + t.Errorf("status = %v, want removed", payload["status"]) + } +} + +func TestDispatch_ItemUnblock_MissingLinkSurfacesError(t *testing.T) { + // Empty links list → the dispatcher must NOT fall back to + // deleting something else; it returns an IsError tool result. + items := map[string]itemPrefetch{ + "TASK-5": {ID: "id-5", Slug: "task-5"}, + "TASK-8": {ID: "id-8", Slug: "task-8"}, + } + deleteCount := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/task-5/links", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/links/", func(_ http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodDelete { + deleteCount++ + } + }) + mux.HandleFunc("/api/v1/workspaces/docapp/items/", itemPrefetchHandler(t, items)) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "source_ref": "TASK-5", "target_ref": "TASK-8", + }) + res, err := d.Dispatch(ctx, []string{"item", "unblock"}, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError when no matching link found; got %#v", res) + } + if deleteCount != 0 { + t.Errorf("DELETE must not run when no link matches; ran %d times", deleteCount) + } +} + +// --- Read-only link queries --- + +func TestDispatch_ItemDeps_ReturnsRawLinksArray(t *testing.T) { + // `deps --format json` returns the raw `/links` array. The + // dispatcher's parity contract says we match — any post- + // processing belongs in agent-side code or in `related`/ + // `implemented-by` (which DO post-process per the CLI). + gotPath := "" + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5/links", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected", http.StatusMethodNotAllowed) + return + } + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"id":"l1","link_type":"blocks","source_id":"a","target_id":"b"}]`)) + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + }) + res, err := d.Dispatch(ctx, []string{"item", "deps"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if gotPath != "/api/v1/workspaces/docapp/items/TASK-5/links" { + t.Errorf("path = %q", gotPath) + } + links, ok := res.StructuredContent.([]any) + if !ok { + t.Fatalf("deps result not a JSON array: %#v", res.StructuredContent) + } + if len(links) != 1 { + t.Errorf("expected 1 link in deps result; got %d", len(links)) + } +} + +func TestDispatch_ItemRelated_ReturnsGroupedShape(t *testing.T) { + // CLI `--format json` for `related` returns + // {item_ref, item_title, collection, group_count, groups}. + // The dispatcher must mirror that — Codex review caught the + // raw-links shape as a divergence. + itemSlug := "task-5" + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "id":"id-task-5", + "slug":"task-5", + "title":"Fix OAuth", + "collection_slug":"tasks", + "collection_prefix":"TASK", + "item_number":5 + }`)) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/items/"+itemSlug+"/links", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Mix: outgoing blocks (TASK-5 blocks something) + incoming + // implements (something implements TASK-5). + _, _ = w.Write([]byte(`[ + {"id":"l1","link_type":"blocks","source_id":"id-task-5","target_id":"id-other","target_ref":"TASK-9","target_title":"Other","target_collection_slug":"tasks","target_status":"open"}, + {"id":"l2","link_type":"implements","source_id":"id-impl","target_id":"id-task-5","source_ref":"TASK-7","source_title":"Implementer","source_collection_slug":"tasks","source_status":"in-progress"} + ]`)) + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + }) + res, err := d.Dispatch(ctx, []string{"item", "related"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + payload, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("related result not structured: %#v", res.StructuredContent) + } + if payload["item_ref"] != "TASK-5" { + t.Errorf("item_ref = %v, want TASK-5", payload["item_ref"]) + } + if payload["item_title"] != "Fix OAuth" { + t.Errorf("item_title = %v", payload["item_title"]) + } + if payload["collection"] != "tasks" { + t.Errorf("collection = %v", payload["collection"]) + } + groups, ok := payload["groups"].([]any) + if !ok { + t.Fatalf("groups not an array: %#v", payload["groups"]) + } + // Two groups expected: blocks (outgoing) + implemented_by (incoming). + gotKeys := map[string]bool{} + for _, g := range groups { + gm, _ := g.(map[string]any) + if k, _ := gm["key"].(string); k != "" { + gotKeys[k] = true + } + } + if !gotKeys["blocks"] || !gotKeys["implemented_by"] { + t.Errorf("expected blocks + implemented_by groups; got %v", gotKeys) + } +} + +func TestDispatch_ItemImplementedBy_FiltersIncomingOnly(t *testing.T) { + // `implemented-by` returns ONLY incoming `implements` links — + // outgoing implements describe what THIS item implements (not + // what implements it). Filtering at the dispatcher matches the + // CLI's incomingImplementedBy helper. + itemSlug := "idea-12" + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/IDEA-12", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "id":"id-idea-12", + "slug":"idea-12", + "title":"Real-time collab", + "collection_slug":"ideas", + "collection_prefix":"IDEA", + "item_number":12 + }`)) + }) + mux.HandleFunc("/api/v1/workspaces/docapp/items/"+itemSlug+"/links", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[ + {"id":"l1","link_type":"implements","source_id":"id-task-7","target_id":"id-idea-12","source_ref":"TASK-7","source_title":"Build it"}, + {"id":"l2","link_type":"implements","source_id":"id-task-8","target_id":"id-idea-12","source_ref":"TASK-8","source_title":"Build it more"}, + {"id":"l3","link_type":"implements","source_id":"id-idea-12","target_id":"id-other","target_ref":"OTHER-1","target_title":"Outgoing"}, + {"id":"l4","link_type":"blocks","source_id":"id-idea-12","target_id":"id-task-9","target_ref":"TASK-9"} + ]`)) + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "IDEA-12", + }) + res, err := d.Dispatch(ctx, []string{"item", "implemented-by"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + payload, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("result not structured: %#v", res.StructuredContent) + } + if payload["item_ref"] != "IDEA-12" { + t.Errorf("item_ref = %v", payload["item_ref"]) + } + count, _ := payload["count"].(float64) + if count != 2 { + t.Errorf("count = %v, want 2 (outgoing implements + non-implements links filtered out)", count) + } + results, ok := payload["results"].([]any) + if !ok || len(results) != 2 { + t.Fatalf("results not a 2-entry array: %#v", payload["results"]) + } + gotRefs := map[string]bool{} + for _, r := range results { + rm, _ := r.(map[string]any) + if ref, _ := rm["ref"].(string); ref != "" { + gotRefs[ref] = true + } + } + if !gotRefs["TASK-7"] || !gotRefs["TASK-8"] { + t.Errorf("expected TASK-7 + TASK-8 as incoming implementers; got %v", gotRefs) + } +} + +func TestDispatch_ItemDeps_RequiresWorkspaceAndRef(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not be called when input incomplete"), + UserResolver: fixedUserResolver(&models.User{ID: "u"}), + } + for _, missing := range []string{"workspace", "ref"} { + t.Run("missing-"+missing, func(t *testing.T) { + input := map[string]any{"workspace": "ws", "ref": "TASK-1"} + delete(input, missing) + ctx := WithDispatchInput(context.Background(), input) + res, err := d.Dispatch(ctx, []string{"item", "deps"}, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError when %s missing", missing) + } + }) + } +} + +// --- Slug-based --role resolution --- + +func TestResolveRoleSlug_PassesThroughWhenMissing(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not call agent-roles when role absent"), + UserResolver: fixedUserResolver(&models.User{ID: "u"}), + } + out, err := d.resolveRoleSlug(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "ws", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if _, has := out["agent_role_id"]; has { + t.Errorf("output should not introduce agent_role_id when role absent; got %v", out) + } +} + +func TestResolveRoleSlug_ResolvesByExactSlug(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/implementer", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"role-uuid-imp","slug":"implementer"}`)) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + + out, err := d.resolveRoleSlug(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "docapp", + "role": "implementer", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if out["agent_role_id"] != "role-uuid-imp" { + t.Errorf("expected role-uuid-imp; got %v", out["agent_role_id"]) + } + if _, present := out["role"]; present { + t.Errorf("role key should be removed after resolution; got %v", out) + } +} + +func TestResolveRoleSlug_ExplicitIDWins(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not call agent-roles when explicit ID present"), + UserResolver: fixedUserResolver(&models.User{ID: "u"}), + } + out, err := d.resolveRoleSlug(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "docapp", + "role": "implementer", + "agent_role_id": "explicit-uuid", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if out["agent_role_id"] != "explicit-uuid" { + t.Errorf("expected explicit ID to win; got %v", out) + } + if _, present := out["role"]; present { + t.Errorf("role key should be cleared even when ID-only path runs: %v", out) + } +} + +func TestResolveRoleSlug_404SurfacesAsClearError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/ghost", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"not found"}`)) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + + _, err := d.resolveRoleSlug(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "docapp", "role": "ghost", + }) + if err == nil { + t.Fatalf("expected error for unknown role slug") + } + if !strings.Contains(err.Error(), `"ghost"`) { + t.Errorf("error should mention the unmatched slug; got %v", err) + } +} + +func TestDispatch_PreprocessesRoleForItemCreate(t *testing.T) { + // End-to-end: role: "implementer" → resolved → mapItemCreate + // sees agent_role_id and the create POST carries the UUID at + // the top level (not in fields). + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/implementer", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"role-uuid-imp","slug":"implementer"}`)) + }) + mux.Handle("/api/v1/workspaces/docapp/collections/tasks/items", captured) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "collection": "tasks", + "title": "Fix oauth", + "role": "implementer", + }) + res, err := d.Dispatch(ctx, []string{"item", "create"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v\n%s", err, captured.lastBody) + } + if body["agent_role_id"] != "role-uuid-imp" { + t.Errorf("create body did not carry resolved role id: %v", body) + } + // And NOT inside fields blob — fields belongs to category / + // status / priority / parent + custom KVPs only. + if fields, _ := body["fields"].(string); fields != "" { + var f map[string]any + _ = json.Unmarshal([]byte(fields), &f) + if _, present := f["agent_role_id"]; present { + t.Errorf("agent_role_id should not be in fields blob: %v", f) + } + } +} + +func TestDispatch_PreprocessRoleFailureSurfacesAsToolError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/ghost", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"not found"}`)) + }) + createCount := 0 + mux.HandleFunc("/api/v1/workspaces/docapp/collections/tasks/items", func(_ http.ResponseWriter, _ *http.Request) { + createCount++ + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "collection": "tasks", "title": "x", + "role": "ghost", + }) + res, err := d.Dispatch(ctx, []string{"item", "create"}, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError when role resolution fails; got %#v", res) + } + if createCount != 0 { + t.Errorf("create handler must not run after role resolution failure; ran %d times", createCount) + } +} + +func TestDispatch_RolePreprocessSkippedForCommandsNotInAllowlist(t *testing.T) { + // `item show` doesn't take --role. Even if the schema cache + // happens to carry a stale role, the dispatcher must not call + // the agent-roles endpoint just to throw the value away. + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/agent-roles/", func(_ http.ResponseWriter, _ *http.Request) { + t.Errorf("agent-roles endpoint must not be called for item.show") + }) + mux.Handle("/api/v1/workspaces/docapp/items/TASK-5", captured) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + "role": "implementer", // ignored + }) + res, err := d.Dispatch(ctx, []string{"item", "show"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } +} + +// --- Integration: drive link commands against the real *server.Server --- + +// TestHTTPHandlerDispatcher_Integration_ItemLinkLifecycle exercises +// the full link-command surface end-to-end against a real handler +// chain + SQLite store. Catches regressions in: +// +// - URL/body asymmetry (block vs blocked-by) +// - Canonical link-type wire form (split_from vs split-from) +// - Read-after-write: deps must surface the freshly-created link +// - Delete-by-ID: unblock must locate the right link by source/ +// target/type and delete it without the agent ever seeing the ID +// +// The unit tests above stub the handler; this is the only test that +// would catch a regression in handler shape, model schema, or +// middleware ordering. +func TestHTTPHandlerDispatcher_Integration_ItemLinkLifecycle(t *testing.T) { + srv, st := newPadServer(t) + + // Bootstrap workspace + owner. + wsRec := doJSONReq(t, srv, http.MethodPost, "/api/v1/workspaces", + map[string]any{"name": "DocApp"}) + if wsRec.Code != http.StatusCreated { + t.Fatalf("create workspace: %d %s", wsRec.Code, wsRec.Body.String()) + } + var ws models.Workspace + if err := json.Unmarshal(wsRec.Body.Bytes(), &ws); err != nil { + t.Fatalf("decode workspace: %v", err) + } + owner, err := st.CreateUser(models.UserCreate{Email: "dave@example.com", Name: "Dave", Password: "x"}) + if err != nil { + t.Fatalf("create owner: %v", err) + } + if err := st.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatalf("add owner: %v", err) + } + + d := &HTTPHandlerDispatcher{Handler: srv, UserResolver: fixedUserResolver(owner)} + + create := func(title string) string { + t.Helper() + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "collection": "tasks", "title": title, + }) + res, err := d.Dispatch(ctx, []string{"item", "create"}, nil) + if err != nil || res.IsError { + t.Fatalf("create %q: err=%v IsError=%v: %#v", title, err, res != nil && res.IsError, res) + } + m, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("create result not structured: %#v", res.StructuredContent) + } + ref, _ := m["ref"].(string) + if ref == "" { + t.Fatalf("create %q missing ref: %#v", title, m) + } + return ref + } + + a := create("A") + b := create("B") + + // `item block A B` → A blocks B. The dispatcher must resolve + // both refs, post the link with A as URL slug + B as target_id. + blockCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "source_ref": a, "target_ref": b, + }) + blockRes, err := d.Dispatch(blockCtx, []string{"item", "block"}, nil) + if err != nil || blockRes.IsError { + t.Fatalf("block: err=%v IsError=%v: %#v", err, blockRes != nil && blockRes.IsError, blockRes) + } + + // `item deps A` should surface the freshly-created blocking link. + depsCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "ref": a, + }) + depsRes, err := d.Dispatch(depsCtx, []string{"item", "deps"}, nil) + if err != nil || depsRes.IsError { + t.Fatalf("deps: err=%v IsError=%v: %#v", err, depsRes != nil && depsRes.IsError, depsRes) + } + links, ok := depsRes.StructuredContent.([]any) + if !ok { + t.Fatalf("deps result not a JSON array: %#v", depsRes.StructuredContent) + } + if len(links) != 1 { + t.Fatalf("expected 1 link, got %d: %#v", len(links), links) + } + link, _ := links[0].(map[string]any) + if link["link_type"] != "blocks" { + t.Errorf("link type = %v, want blocks", link["link_type"]) + } + + // `item unblock A B` must locate the link and DELETE it. After + // the delete, deps should be empty. + unblockCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "source_ref": a, "target_ref": b, + }) + unblockRes, err := d.Dispatch(unblockCtx, []string{"item", "unblock"}, nil) + if err != nil || unblockRes.IsError { + t.Fatalf("unblock: err=%v IsError=%v: %#v", err, unblockRes != nil && unblockRes.IsError, unblockRes) + } + + depsAfterCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "ref": a, + }) + depsAfterRes, err := d.Dispatch(depsAfterCtx, []string{"item", "deps"}, nil) + if err != nil || depsAfterRes.IsError { + t.Fatalf("deps after unblock: err=%v IsError=%v: %#v", err, depsAfterRes != nil && depsAfterRes.IsError, depsAfterRes) + } + if linksAfter, ok := depsAfterRes.StructuredContent.([]any); !ok || len(linksAfter) != 0 { + t.Errorf("expected empty links after unblock, got %#v", depsAfterRes.StructuredContent) + } + + // `item supersedes new old` exercises the lineage path against + // the real handler — the canonical wire form of the link type + // MUST be `supersedes` (not `supersede` or any other variant) + // or the store rejects the create. + supersedesCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, "new_ref": a, "old_ref": b, + }) + supRes, err := d.Dispatch(supersedesCtx, []string{"item", "supersedes"}, nil) + if err != nil || supRes.IsError { + t.Fatalf("supersedes: err=%v IsError=%v: %#v", err, supRes != nil && supRes.IsError, supRes) + } +} + +// --- noRemoteEquivalent --- + +func TestDispatch_NoRemoteEquivalentReturnsStableError(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "handler must not run for CLI-only commands"), + UserResolver: fixedUserResolver(&models.User{ID: "u"}), + } + cmds := [][]string{ + {"agent", "status"}, + {"mcp", "status"}, + {"mcp", "uninstall"}, + {"server", "info"}, + {"server", "open"}, + {"workspace", "link"}, + {"workspace", "switch"}, + {"workspace", "context"}, + } + for _, cmd := range cmds { + t.Run(strings.Join(cmd, "."), func(t *testing.T) { + ctx := WithDispatchInput(context.Background(), map[string]any{}) + res, err := d.Dispatch(ctx, cmd, nil) + if err != nil { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Fatalf("expected IsError for CLI-only command; got %#v", res) + } + if !containsToolText(res, "no remote equivalent") { + t.Errorf("error should call out CLI-only nature; got %#v", res) + } + }) + } +} diff --git a/internal/mcp/dispatch_http_test.go b/internal/mcp/dispatch_http_test.go index 9beca565..a1d6a4b6 100644 --- a/internal/mcp/dispatch_http_test.go +++ b/internal/mcp/dispatch_http_test.go @@ -227,21 +227,28 @@ func TestMapItemCreate_NormalizesCollectionAliases(t *testing.T) { } } -func TestMapItemCreate_RejectsUnsupportedRole(t *testing.T) { - // --role still requires slug → role-ID resolution we haven't - // built yet (item.update lands first; --role rolls in with the - // next route-table expansion). Reject loudly so agents don't - // silently lose role assignments. - _, _, _, err := mapItemCreate(map[string]any{ +func TestMapItemCreate_PassesThroughResolvedAgentRoleID(t *testing.T) { + // Slug → ID resolution moved up to Dispatch in TASK-968 (the + // preprocess step rewrites `role: ` to `agent_role_id: + // ` via the agent-roles endpoint). The mapper itself just + // trusts the resolved input. This test asserts that the + // pass-through for `agent_role_id` keeps working — the Dispatch- + // level preprocess can't actually run without a Handler, so the + // route table contract is "agent_role_id flows through to the + // payload." + _, _, body, err := mapItemCreate(map[string]any{ "workspace": "ws", "collection": "tasks", "title": "x", - "role": "implementer", + "agent_role_id": "role-uuid-101", }) - if err == nil { - t.Errorf("expected error rejecting --role; got nil") - return + if err != nil { + t.Fatalf("mapItemCreate: %v", err) } - if !strings.Contains(err.Error(), "role") { - t.Errorf("error should mention --role; got %v", err) + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + if payload["agent_role_id"] != "role-uuid-101" { + t.Errorf("agent_role_id should pass through to payload; got %v", payload) } }