diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index edc69592..e33a7d98 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -105,10 +105,37 @@ type RouteMapper func(input map[string]any) (method, path string, body []byte, e // error from Dispatch. var routeTable map[string]RouteMapper +// commandsAcceptingAssignByName is the allowlist of cmdPaths whose +// `--assign ` input should be resolved to a user ID via +// the workspace-members endpoint before the mapper sees it. Other +// commands using an `assign` input pass through unchanged so we +// don't accidentally rewrite a non-assignment use of the same key. +var commandsAcceptingAssignByName = map[string]struct{}{ + "item create": {}, + "item update": {}, + "item list": {}, +} + // Dispatch satisfies the Dispatcher interface. cliArgs are accepted // for interface compatibility but ignored — HTTPHandlerDispatcher // reads the structured input attached by the registry via // WithDispatchInput. +// +// Flow: +// +// 1. Validate dispatcher config (Handler + UserResolver wired). +// 2. Resolve the requesting user from the MCP context. +// 3. Pull the structured input from context. +// 4. Preprocess the input — resolve `--assign ` to +// `assigned_user_id ` for the commands that accept it +// (TASK-967). Failures here surface as IsError tool results so +// agents see the resolution error message. +// 5. Special-case routes that need read-modify-write semantics +// (item.update merges existing fields with the update payload — +// the handler treats fields as a complete replacement, so the +// dispatcher does the merge). +// 6. Otherwise, look up a RouteMapper in routeTable, build the +// synthesized request, and execute through the handler chain. func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []string) (*mcp.CallToolResult, error) { if d.Handler == nil { return mcp.NewToolResultError("HTTPHandlerDispatcher: Handler not configured"), nil @@ -118,6 +145,39 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin } cmdKey := strings.Join(cmdPath, " ") + user := d.UserResolver(ctx) + if user == nil { + return mcp.NewToolResultErrorf("%s: no authenticated user in context", cmdKey), nil + } + + input := DispatchInputFromContext(ctx) + if input == nil { + // Defensive: registry always attaches input. Empty map keeps + // the mapper from panicking on nil. + input = map[string]any{} + } + + // Preprocess input: resolve --assign name → assigned_user_id for + // commands that accept the shorthand. Mappers downstream see only + // the resolved UUID; agents that already pass an ID via + // `--field assigned_user_id=` are unaffected. + if _, ok := commandsAcceptingAssignByName[cmdKey]; ok { + var err error + input, err = d.resolveAssignName(ctx, user, input) + if err != nil { + return mcp.NewToolResultErrorf("%s: resolve --assign: %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 + // table only carries pure mappers. + switch cmdKey { + case "item update": + return d.dispatchItemUpdate(ctx, input, user) + } + routes := d.Routes if routes == nil { routes = routeTable @@ -134,36 +194,63 @@ func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []strin ), nil } - input := DispatchInputFromContext(ctx) - if input == nil { - // Defensive: registry always attaches input. Empty map keeps - // the mapper from panicking on nil. - input = map[string]any{} - } - method, urlPath, body, err := mapper(input) if err != nil { return mcp.NewToolResultErrorf("%s: %s", cmdKey, err.Error()), nil } - user := d.UserResolver(ctx) - if user == nil { - return mcp.NewToolResultErrorf("%s: no authenticated user in context", cmdKey), nil - } + return d.executeRequest(ctx, cmdKey, user, method, urlPath, body) +} - req, err := buildHTTPRequest(ctx, method, urlPath, body, user) +// executeRequest builds + serves + packages a single HTTP request +// against the wrapped handler. Pulled out of Dispatch so the +// special-case methods (dispatchItemUpdate, future RMW commands) can +// reuse the same auth-context + recorder + response-shaping path. +func (d *HTTPHandlerDispatcher) executeRequest( + ctx context.Context, + cmdKey string, + user *models.User, + method, urlPath string, + body []byte, +) (*mcp.CallToolResult, error) { + req, err := d.buildAuthedRequest(ctx, method, urlPath, body, user) if err != nil { return mcp.NewToolResultErrorf("%s: build request: %s", cmdKey, err.Error()), nil } - if d.Apply != nil { - req = d.Apply(req) - } rec := httptest.NewRecorder() d.Handler.ServeHTTP(rec, req) return packageHTTPResponse(cmdKey, rec.Result()) } +// buildAuthedRequest constructs an in-process HTTP request against +// the wrapped handler with the user attached via WithCurrentUser AND +// any caller-supplied decoration (d.Apply) applied. Used by both +// the main dispatch path and the in-handler prefetches +// (dispatchItemUpdate's GET, lookupAssigneeID's members fetch) so +// token-scope context attached via Apply applies uniformly to every +// synthesized request — no scope bypass on the prefetches. +// +// Codex review #345 round 1 caught the inconsistency: if an OAuth +// middleware attached workspace-allow-list context via Apply, the +// prefetches were bypassing that and could read members / items +// outside the allowed set during resolution. +func (d *HTTPHandlerDispatcher) buildAuthedRequest( + ctx context.Context, + method, urlPath string, + body []byte, + user *models.User, +) (*http.Request, error) { + req, err := buildHTTPRequest(ctx, method, urlPath, body, user) + if err != nil { + return nil, err + } + if d.Apply != nil { + req = d.Apply(req) + } + return req, nil +} + // buildHTTPRequest constructs the in-process request, attaching the // user via the exported server.WithCurrentUser helper so the handler // chain treats the call as authenticated. Pulled out so tests can @@ -305,6 +392,16 @@ func mapItemCreate(input map[string]any) (method, path string, body []byte, err payload[key] = v } } + // Lift recognized column keys out of the fields blob into the + // top-level payload. The MCP tool schema (auto-generated from + // cmdhelp) doesn't expose `agent_role_id` or `assigned_user_id` + // as top-level inputs — only `--role` and `--assign` — so + // agents reaching for these column writes via the schema- + // visible escape hatch (`--field agent_role_id=`) would + // otherwise have the value sit inert inside the fields JSON + // instead of going to the column the handler writes to. + // (Codex review #345 round 3.) + liftFieldsToColumns(fields, payload) if len(fields) > 0 { fb, mErr := json.Marshal(fields) if mErr != nil { @@ -318,18 +415,39 @@ func mapItemCreate(input map[string]any) (method, path string, body []byte, err payload["tags"] = v } - // Reject role/assign with a clear error rather than silently - // dropping them — full support requires resolving names → IDs - // and is being tracked as a follow-up to TASK-965. - for _, unsupported := range []string{"assign", "role"} { - if v, ok := input[unsupported]; ok { - if s, ok := v.(string); ok && s != "" { - return "", "", nil, fmt.Errorf( - "--%s is not yet supported by HTTPHandlerDispatcher; "+ - "resolution from name to ID lands in a follow-up to TASK-965", - unsupported, - ) - } + // `--assign` resolution lives at the dispatcher level (TASK-967): + // Dispatch's preprocess step rewrites it to `assigned_user_id` + // before the mapper runs, so by the time we get here a name has + // already been resolved to a UUID. If the resolved ID is set, + // pass it through to the handler. + if v, ok := input["assigned_user_id"].(string); ok && v != "" { + payload["assigned_user_id"] = v + } + // `agent_role_id` (UUID) passes through directly to the + // ItemCreate column. Agents that know the role's UUID (e.g. + // from a prior `role list` call) can set it without round- + // tripping through `--role` slug resolution. + 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.") } } @@ -360,6 +478,50 @@ func isStringType(v any) bool { return ok } +// columnFieldKeys is the set of fields-blob keys the dispatcher +// recognizes as actually being column writes on the underlying +// item record. When agents pass these via `--field key=value` (the +// only schema-visible path until cmdhelp surfaces them as their own +// flags), the dispatcher lifts them out of the fields JSON into the +// top-level payload so the handler writes the column rather than +// stuffing the value inside the JSON blob. +// +// Adding a key here is a behavioural extension — agents now get +// column writes via --field instead of the inert fields-blob +// no-op. Keep the list short; a real flag in cmdhelp is preferable +// long-term (the eventual TASK-968 follow-up should add named +// inputs for these). +var columnFieldKeys = []string{ + "agent_role_id", + "assigned_user_id", +} + +// liftFieldsToColumns scans fields for keys that map 1:1 to columns +// on the item record and moves them into payload at the top level. +// Mutates both maps. Caller-supplied top-level values win — so an +// agent that already passed `agent_role_id` directly (e.g. via a +// future named flag) doesn't get clobbered by a stray --field entry. +func liftFieldsToColumns(fields, payload map[string]any) { + for _, key := range columnFieldKeys { + v, ok := fields[key] + if !ok { + continue + } + // Always remove from the fields blob — even if payload + // already has it. The fields blob is the wrong home and + // leaving the duplicate would invite divergence between the + // JSON value and the column. + delete(fields, key) + if _, alreadyTopLevel := payload[key]; alreadyTopLevel { + continue + } + if s, ok := v.(string); ok && s == "" { + continue + } + payload[key] = v + } +} + // parseFieldKVP normalizes the --field flag's various wire shapes // (single string, []string, []any) into a `key→value` map. Empty / // invalid entries are skipped silently to match the CLI's permissive diff --git a/internal/mcp/dispatch_http_advanced.go b/internal/mcp/dispatch_http_advanced.go new file mode 100644 index 00000000..332a496f --- /dev/null +++ b/internal/mcp/dispatch_http_advanced.go @@ -0,0 +1,305 @@ +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" +) + +// resolveAssignName rewrites a `--assign ` input into +// `assigned_user_id ` by hitting the workspace-members +// endpoint and finding a matching user. Mirrors the CLI's behaviour +// in cmd/pad/main.go's itemCreateCmd / itemUpdateCmd / itemListCmd — +// without this resolution, agents passing human-friendly assignee +// values would silently get empty results (the store filters by +// `i.assigned_user_id = ?` UUID, no name fallback). +// +// Returns the input map with `assign` replaced by `assigned_user_id` +// when a match is found, or unchanged when `assign` is missing / +// empty. Mismatches return a clear error so agents know to pass a +// different name. +// +// The returned map is always a fresh map — the caller's reference +// isn't mutated, matching the no-mutation contract of the rest of +// the dispatcher. +func (d *HTTPHandlerDispatcher) resolveAssignName( + ctx context.Context, + user *models.User, + input map[string]any, +) (map[string]any, error) { + rawAssign, present := input["assign"] + if !present { + return input, nil + } + assign, _ := rawAssign.(string) + if assign == "" { + return input, nil + } + // Already-resolved? If the caller used `--field assigned_user_id=` + // that's a separate input key — we don't touch it. If the caller + // passed both `assign` and `assigned_user_id`, the explicit ID + // wins; drop the assign value to avoid the resolution lookup. + out := cloneStringMap(input) + if existingID, _ := out["assigned_user_id"].(string); existingID != "" { + delete(out, "assign") + return out, nil + } + + workspace, _ := input["workspace"].(string) + if workspace == "" { + return nil, fmt.Errorf("workspace is required to resolve --assign") + } + + userID, err := d.lookupAssigneeID(ctx, user, workspace, assign) + if err != nil { + return nil, err + } + out["assigned_user_id"] = userID + delete(out, "assign") + return out, 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. +// +// Errors: +// +// - underlying handler returns non-2xx → wrapped error with body. +// - response shape doesn't match expected {members:[...]} → error. +// - no member matches → "no workspace member matches --assign %q". +func (d *HTTPHandlerDispatcher) lookupAssigneeID( + ctx context.Context, + user *models.User, + workspace string, + assign string, +) (string, error) { + path := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/members" + // Goes through buildAuthedRequest so d.Apply (the OAuth-scope + // hook) sees this prefetch the same as a top-level dispatch — + // no scope bypass during assignee resolution. + req, err := d.buildAuthedRequest(ctx, http.MethodGet, path, nil, user) + if err != nil { + return "", fmt.Errorf("build members 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 "", fmt.Errorf("list workspace members: %d %s", rec.Code, body) + } + + // Response shape: {"members":[{user_id, user_name, user_email, ...}, ...], "invitations":[...]} + var resp struct { + Members []struct { + UserID string `json:"user_id"` + UserName string `json:"user_name"` + UserEmail string `json:"user_email"` + } `json:"members"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + return "", fmt.Errorf("parse members response: %w", err) + } + + for _, m := range resp.Members { + if strings.EqualFold(m.UserName, assign) || strings.EqualFold(m.UserEmail, assign) { + return m.UserID, nil + } + } + return "", fmt.Errorf("no workspace member matches --assign %q", assign) +} + +// dispatchItemUpdate handles `pad item update ` with full CLI +// parity, including the read-modify-write merge of the fields JSON. +// +// The handler at handleUpdateItem treats input.Fields as a complete +// replacement (json_extract-friendly), but the CLI does a GET first +// to read existing fields, merges in new --status / --priority / +// --field overrides, then PATCHes the merged result. Without this +// dispatch path, an MCP `item.update --status done` would erase +// every other field the schema set — Codex caught the equivalent +// shape regression on item.create in PR #343. +// +// Sequence: +// +// 1. GET /api/v1/workspaces/{ws}/items/{ref} — read current state. +// 2. Merge: existing.fields + input.{status, priority, category, +// parent} + parsed --field key=value pairs. Last-write-wins per +// key (matches CLI; --field can override --status). +// 3. PATCH /api/v1/workspaces/{ws}/items/{ref} with the merged +// payload. +// +// Returns the PATCH response packaged like any other dispatch result +// (structured JSON if 2xx + JSON body, IsError-flagged if non-2xx). +func (d *HTTPHandlerDispatcher) dispatchItemUpdate( + ctx context.Context, + input map[string]any, + user *models.User, +) (*mcp.CallToolResult, error) { + const cmdKey = "item update" + + workspace, _ := input["workspace"].(string) + ref, _ := input["ref"].(string) + if workspace == "" { + return mcp.NewToolResultErrorf("%s: workspace is required", cmdKey), nil + } + if ref == "" { + return mcp.NewToolResultErrorf("%s: ref is required", cmdKey), nil + } + + 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 + } + + // Step 1: GET the existing item so we can read fields for the + // read-modify-write merge. If the GET fails (item not found, + // permission error, …), surface that to the caller — there's no + // point trying to PATCH an item we can't read. + // + // Goes through buildAuthedRequest so d.Apply (the OAuth-scope + // hook) sees this prefetch the same as a top-level dispatch. + prefetchReq, err := d.buildAuthedRequest(ctx, http.MethodGet, itemPath, nil, user) + if err != nil { + return mcp.NewToolResultErrorf("%s: build prefetch request: %s", cmdKey, err.Error()), nil + } + prefetchRec := httptest.NewRecorder() + d.Handler.ServeHTTP(prefetchRec, prefetchReq) + if prefetchRec.Code >= 400 { + // Mirror the CLI's "not found" UX — the handler's 404 body + // already contains a clear message; package it the same way + // any other tool error would be packaged. + return packageHTTPResponse(cmdKey, prefetchRec.Result()) + } + var existing struct { + Fields string `json:"fields"` + } + if err := json.Unmarshal(prefetchRec.Body.Bytes(), &existing); err != nil { + return mcp.NewToolResultErrorf("%s: parse current item: %s", cmdKey, err.Error()), nil + } + + // Step 2: Build the PATCH payload. + payload := map[string]any{} + for _, key := range []string{"title", "content", "comment", "tags"} { + if v, ok := input[key].(string); ok && v != "" { + payload[key] = v + } + } + if v, ok := input["assigned_user_id"].(string); ok && v != "" { + payload["assigned_user_id"] = v + } + if v, ok := input["agent_role_id"].(string); ok && v != "" { + payload["agent_role_id"] = v + } + if b, ok := input["pinned"].(bool); ok { + payload["pinned"] = b + } + + // Field merging — the actual reason this command needs a custom + // dispatcher rather than a routeSpec entry. Match the CLI's + // last-write-wins precedence: existing fields, then named flags + // (status / priority / category / parent), then --field entries. + if hasFieldChanges(input) { + merged := map[string]any{} + if existing.Fields != "" && existing.Fields != "{}" { + if err := json.Unmarshal([]byte(existing.Fields), &merged); err != nil { + return mcp.NewToolResultErrorf( + "%s: parse existing fields JSON: %s", cmdKey, err.Error()), nil + } + } + for _, key := range []string{"status", "priority", "category", "parent"} { + if v, ok := input[key].(string); ok && v != "" { + merged[key] = v + } + } + if rawFields, ok := input["field"]; ok { + extra, err := parseFieldKVP(rawFields) + if err != nil { + return mcp.NewToolResultErrorf("%s: parse --field: %s", cmdKey, err.Error()), nil + } + for k, v := range extra { + merged[k] = v + } + } + // Lift recognized column keys (agent_role_id, assigned_user_id) + // out of the merged fields blob onto the top-level payload so + // the handler writes the column instead of stuffing the value + // inert in the JSON. Same shape mapItemCreate uses; matches + // the workaround the --role rejection points at. + liftFieldsToColumns(merged, payload) + fieldsJSON, err := json.Marshal(merged) + if err != nil { + return mcp.NewToolResultErrorf("%s: encode merged fields: %s", cmdKey, err.Error()), nil + } + fieldsStr := string(fieldsJSON) + payload["fields"] = fieldsStr + } + + body, err := json.Marshal(payload) + if err != nil { + return mcp.NewToolResultErrorf("%s: encode body: %s", cmdKey, err.Error()), nil + } + + // Step 3: PATCH. + return d.executeRequest(ctx, cmdKey, user, http.MethodPatch, itemPath, body) +} + +// hasFieldChanges reports whether the input has any value that +// should trigger field-merging on update. Mirrors the CLI's check +// at cmd/pad/main.go itemUpdateCmd around the `hasFieldChanges` +// boolean — without this guard, dispatching `item update TASK-1 +// --content "x"` would do an unnecessary GET-merge-PATCH of +// fields, churning the audit log entry for no reason. +func hasFieldChanges(input map[string]any) bool { + for _, key := range []string{"status", "priority", "category", "parent"} { + if v, ok := input[key].(string); ok && v != "" { + return true + } + } + if rawFields, ok := input["field"]; ok && rawFields != nil { + switch x := rawFields.(type) { + case string: + return x != "" + case []any: + return len(x) > 0 + case []string: + return len(x) > 0 + } + } + return false +} diff --git a/internal/mcp/dispatch_http_advanced_test.go b/internal/mcp/dispatch_http_advanced_test.go new file mode 100644 index 00000000..67902547 --- /dev/null +++ b/internal/mcp/dispatch_http_advanced_test.go @@ -0,0 +1,750 @@ +package mcp + +import ( + "context" + "encoding/json" + "io" + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// --- resolveAssignName --- + +func TestResolveAssignName_PassesThroughWhenMissing(t *testing.T) { + d := &HTTPHandlerDispatcher{Handler: errorHandler(t, "must not call members"), UserResolver: fixedUserResolver(&models.User{ID: "u"})} + out, err := d.resolveAssignName(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "ws", "title": "x", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if _, has := out["assign"]; has { + t.Errorf("output should not introduce an assign key; got %v", out) + } +} + +func TestResolveAssignName_PassesThroughWhenAssignEmpty(t *testing.T) { + d := &HTTPHandlerDispatcher{Handler: errorHandler(t, "must not call members"), UserResolver: fixedUserResolver(&models.User{ID: "u"})} + out, err := d.resolveAssignName(context.Background(), &models.User{ID: "u"}, map[string]any{ + "workspace": "ws", "assign": "", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if v, _ := out["assigned_user_id"].(string); v != "" { + t.Errorf("expected no resolution for empty assign; got %v", out) + } +} + +func TestResolveAssignName_ResolvesByName(t *testing.T) { + h := membersHandler(t, []memberRow{ + {UserID: "u1", UserName: "Dave", UserEmail: "dave@example.com"}, + {UserID: "u2", UserName: "Alice", UserEmail: "alice@example.com"}, + }) + d := &HTTPHandlerDispatcher{Handler: h, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + + out, err := d.resolveAssignName(context.Background(), &models.User{ID: "caller"}, map[string]any{ + "workspace": "docapp", "assign": "Dave", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if out["assigned_user_id"] != "u1" { + t.Errorf("expected u1, got %v", out["assigned_user_id"]) + } + if _, present := out["assign"]; present { + t.Errorf("assign key should be removed after resolution: %v", out) + } +} + +func TestResolveAssignName_ResolvesByEmailCaseInsensitive(t *testing.T) { + h := membersHandler(t, []memberRow{ + {UserID: "u1", UserName: "Dave", UserEmail: "dave@example.com"}, + }) + d := &HTTPHandlerDispatcher{Handler: h, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + + out, err := d.resolveAssignName(context.Background(), &models.User{ID: "caller"}, map[string]any{ + "workspace": "docapp", "assign": "DAVE@example.com", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if out["assigned_user_id"] != "u1" { + t.Errorf("expected u1 from case-insensitive email match, got %v", out["assigned_user_id"]) + } +} + +func TestResolveAssignName_ErrorsWhenNoMatch(t *testing.T) { + h := membersHandler(t, []memberRow{{UserID: "u1", UserName: "Alice", UserEmail: "alice@example.com"}}) + d := &HTTPHandlerDispatcher{Handler: h, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + + _, err := d.resolveAssignName(context.Background(), &models.User{ID: "caller"}, map[string]any{ + "workspace": "docapp", "assign": "Bob", + }) + if err == nil { + t.Fatalf("expected error for unmatched assignee") + } + if !strings.Contains(err.Error(), `"Bob"`) { + t.Errorf("error should mention the unmatched name; got %v", err) + } +} + +func TestResolveAssignName_ExplicitIDWins(t *testing.T) { + // If the caller passes both --assign Dave and an explicit + // assigned_user_id, the explicit ID wins and we skip the + // members-lookup entirely. + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not call members when assigned_user_id is explicit"), + UserResolver: fixedUserResolver(&models.User{ID: "caller"}), + } + out, err := d.resolveAssignName(context.Background(), &models.User{ID: "caller"}, map[string]any{ + "workspace": "docapp", "assign": "Dave", "assigned_user_id": "explicit-uuid", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if out["assigned_user_id"] != "explicit-uuid" { + t.Errorf("expected explicit ID to win; got %v", out) + } + if _, present := out["assign"]; present { + t.Errorf("assign key should be cleared even when ID-only path runs: %v", out) + } +} + +func TestResolveAssignName_ErrorsWhenMembersEndpointFails(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"forbidden"}`)) + }) + d := &HTTPHandlerDispatcher{Handler: h, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + + _, err := d.resolveAssignName(context.Background(), &models.User{ID: "caller"}, map[string]any{ + "workspace": "docapp", "assign": "Dave", + }) + if err == nil { + t.Fatalf("expected error when members endpoint returns 403") + } + if !strings.Contains(err.Error(), "403") { + t.Errorf("error should propagate status: %v", err) + } +} + +// --- Dispatch preprocess for assign --- + +func TestDispatch_PreprocessesAssignForItemCreate(t *testing.T) { + // End-to-end through Dispatch: input has --assign Dave; Dispatch + // resolves via members endpoint, mapItemCreate sees the resolved + // assigned_user_id, and the create POST carries the UUID. + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/members", membersHandler(t, []memberRow{ + {UserID: "u-dave", UserName: "Dave", UserEmail: "dave@example.com"}, + }).ServeHTTP) + mux.Handle("/api/v1/workspaces/docapp/collections/tasks/items", captured) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "collection": "tasks", + "title": "Fix oauth", + "assign": "Dave", + }) + 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) + } + if captured.requestCount != 1 { + t.Fatalf("expected exactly 1 captured create request, got %d", captured.requestCount) + } + 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["assigned_user_id"] != "u-dave" { + t.Errorf("create body did not carry resolved user id: %v", body) + } +} + +func TestDispatch_PreprocessAssignFailureSurfacesAsToolError(t *testing.T) { + // When resolution fails (no matching member), Dispatch must + // return an IsError result rather than dispatching the create + // with an empty assignee — silently posting without the + // assignment would be the worst outcome. + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/members", membersHandler(t, []memberRow{ + {UserID: "u-alice", UserName: "Alice", UserEmail: "alice@example.com"}, + }).ServeHTTP) + 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: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "collection": "tasks", "title": "x", + "assign": "Bob", + }) + 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 assignee resolution fails; got %#v", res) + } + if createCount != 0 { + t.Errorf("create handler must not run after preprocess failure; ran %d times", createCount) + } +} + +func TestDispatch_PreprocessSkippedForCommandsNotInAllowlist(t *testing.T) { + // `item show` doesn't take --assign — Dispatch must not call + // the members endpoint just because input happens to carry an + // `assign` key (e.g. from a stale schema cache). + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/members", func(_ http.ResponseWriter, _ *http.Request) { + t.Errorf("members 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: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + "assign": "Dave", // 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) + } +} + +// --- dispatchItemUpdate --- + +func TestDispatchItemUpdate_MergesFieldsWithExisting(t *testing.T) { + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + // Existing item with two fields set. + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "ref":"TASK-5", + "fields":"{\"status\":\"open\",\"priority\":\"medium\",\"category\":\"infra\"}" + }`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5","status":"updated"}`)) + 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", + "status": "in-progress", // overrides existing "open" + "comment": "Started work", // top-level update + "field": []any{"effort=l"}, // adds a new key + }) + 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) + } + if captured.requestCount != 1 { + t.Fatalf("expected 1 PATCH, got %d", captured.requestCount) + } + 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["comment"] != "Started work" { + t.Errorf("comment lost: %v", body) + } + fields := map[string]any{} + if s, ok := body["fields"].(string); ok { + _ = json.Unmarshal([]byte(s), &fields) + } else { + t.Fatalf("fields not a string in body: %v", body) + } + want := map[string]string{ + "status": "in-progress", + "priority": "medium", // existing, preserved + "category": "infra", // existing, preserved + "effort": "l", // newly added via --field + } + for k, v := range want { + if got := fields[k]; got != v { + t.Errorf("merged fields[%q] = %v, want %v", k, got, v) + } + } +} + +func TestDispatchItemUpdate_NoFieldChangesSkipsFieldsMerge(t *testing.T) { + // Updating only top-level keys (title / content / comment) + // without any field-level changes must not include `fields` in + // the PATCH body. Sending a fields object would still go through + // the handler's schema validator (cheap but unnecessary), and a + // no-op update of fields would churn the audit log. + captured := newRequestCapture() + mux := http.NewServeMux() + 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"}`)) + } + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + "comment": "noted", + }) + if _, err := d.Dispatch(ctx, []string{"item", "update"}, nil); err != nil { + t.Fatalf("Dispatch err: %v", err) + } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if _, present := body["fields"]; present { + t.Errorf("fields should be omitted when no field changes; got %v", body) + } +} + +func TestDispatchItemUpdate_SurfacesPrefetch404(t *testing.T) { + // If the GET prefetch fails (item not found), the PATCH must + // not run. The 404 surfaces to the agent as a tool error. + patchCount := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch { + patchCount++ + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"item not found"}`)) + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", "status": "done", + }) + 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 result on 404 prefetch; got %#v", res) + } + if patchCount != 0 { + t.Errorf("PATCH must not run after 404 prefetch; ran %d times", patchCount) + } +} + +func TestDispatchItemUpdate_RequiresWorkspaceAndRef(t *testing.T) { + d := &HTTPHandlerDispatcher{ + Handler: errorHandler(t, "must not reach handler when workspace/ref missing"), + UserResolver: fixedUserResolver(&models.User{ID: "caller"}), + } + for _, missing := range []string{"workspace", "ref"} { + t.Run("missing-"+missing, func(t *testing.T) { + input := map[string]any{"workspace": "ws", "ref": "TASK-1", "status": "done"} + delete(input, missing) + ctx := WithDispatchInput(context.Background(), input) + 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 %s missing; got %#v", missing, res) + } + }) + } +} + +// --- Integration smoke against the real server --- + +// TestHTTPHandlerDispatcher_Integration_ItemUpdateAndAssignResolution +// drives item.create → item.update through the real *server.Server, +// asserting that: +// +// - --assign Dave gets resolved via workspace.members → user UUID, +// and the item lands assigned to Dave. +// - item.update preserves existing fields while updating status — +// i.e. the read-modify-write merge actually keeps the priority + +// category set at create time. +// +// This is the behavioural integration the unit tests can't cover — +// they stub out the handler. Here we run against the real chi +// router + SQLite store so a regression in handler shape, store +// schema, or middleware ordering would fail this test. +func TestHTTPHandlerDispatcher_Integration_ItemUpdateAndAssignResolution(t *testing.T) { + srv, st := newPadServer(t) + + // Bootstrap workspace + two users + memberships. + 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) + } + assignee, err := st.CreateUser(models.UserCreate{Email: "alice@example.com", Name: "Alice", Password: "x"}) + if err != nil { + t.Fatalf("create assignee: %v", err) + } + if err := st.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatalf("add owner: %v", err) + } + if err := st.AddWorkspaceMember(ws.ID, assignee.ID, "editor"); err != nil { + t.Fatalf("add editor: %v", err) + } + + d := &HTTPHandlerDispatcher{ + Handler: srv, + UserResolver: fixedUserResolver(owner), + } + + // Create an item with --assign Alice — exercises Dispatch's + // preprocess + the members-lookup against the real handler. + createCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, + "collection": "tasks", + "title": "Smoke", + "priority": "high", + "category": "infra", + "assign": "Alice", + }) + createRes, err := d.Dispatch(createCtx, []string{"item", "create"}, nil) + if err != nil || createRes.IsError { + t.Fatalf("item create: err=%v IsError=%v: %#v", err, createRes != nil && createRes.IsError, createRes) + } + created, ok := createRes.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("create result not structured: %#v", createRes.StructuredContent) + } + ref, _ := created["ref"].(string) + if ref == "" { + t.Fatalf("created item missing ref: %#v", created) + } + if got, _ := created["assigned_user_id"].(string); got != assignee.ID { + t.Errorf("created item not assigned to Alice; got %q want %q", got, assignee.ID) + } + + // Update only the status. The priority + category set at create + // time MUST survive — that's what the read-modify-write merge + // guarantees vs. the handler treating Fields as a complete + // replacement. + updateCtx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": ws.Slug, + "ref": ref, + "status": "in-progress", + "comment": "Picked up.", + }) + updateRes, err := d.Dispatch(updateCtx, []string{"item", "update"}, nil) + if err != nil || updateRes.IsError { + t.Fatalf("item update: err=%v IsError=%v: %#v", err, updateRes != nil && updateRes.IsError, updateRes) + } + updated, ok := updateRes.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("update result not structured: %#v", updateRes.StructuredContent) + } + fields, _ := updated["fields"].(string) + var fieldMap map[string]any + if err := json.Unmarshal([]byte(fields), &fieldMap); err != nil { + t.Fatalf("decode fields: %v\n%s", err, fields) + } + want := map[string]string{ + "status": "in-progress", + "priority": "high", // pre-existing, must survive RMW + "category": "infra", // pre-existing, must survive RMW + } + for k, v := range want { + if got, _ := fieldMap[k].(string); got != v { + t.Errorf("fields[%q] = %q, want %q (full fields: %v)", k, got, v, fieldMap) + } + } +} + +func TestDispatchItemUpdate_LiftsAgentRoleIDFromFieldKVPToColumn(t *testing.T) { + // Same reachability fix as mapItemCreate: agents passing + // `--field agent_role_id=` (the only schema-visible path + // for setting the column today) get the value lifted onto + // the PATCH body's top-level. Otherwise the value would sit + // inert inside the merged fields JSON and the role assignment + // would no-op (Codex review #345 round 3). + captured := newRequestCapture() + mux := http.NewServeMux() + 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"}`)) + } + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + "field": []any{"agent_role_id=role-uuid-from-field"}, + }) + if _, err := d.Dispatch(ctx, []string{"item", "update"}, nil); err != nil { + t.Fatalf("Dispatch err: %v", err) + } + 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-from-field" { + t.Errorf("agent_role_id not lifted onto PATCH body: %v", body) + } + // And not in the 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 be removed from fields blob: %v", fields) + } +} + +func TestDispatchItemUpdate_PassesThroughAgentRoleID(t *testing.T) { + // Parity with mapItemCreate: agent_role_id (UUID) writes to the + // ItemUpdate column. The --role rejection points agents at this + // path, so it must actually work end-to-end (Codex review #345 + // round 2 caught the misleading error pointing at --field, which + // would have put the value in the fields JSON instead). + captured := newRequestCapture() + mux := http.NewServeMux() + 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":"{}"}`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + } + }) + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + "agent_role_id": "role-uuid-789", + }) + if _, err := d.Dispatch(ctx, []string{"item", "update"}, nil); err != nil { + t.Fatalf("Dispatch err: %v", err) + } + 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-789" { + t.Errorf("agent_role_id not in PATCH body: %v", body) + } + // And NOT inside the fields JSON (the misleading workaround + // would have put it there). + if fields, _ := body["fields"].(string); fields != "" { + t.Errorf("agent_role_id should not be in fields blob: %v", fields) + } +} + +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 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(_ http.ResponseWriter, _ *http.Request) { + prefetchCount++ + }) + 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 { + t.Fatalf("Dispatch err: %v", err) + } + if !res.IsError { + t.Errorf("expected IsError when --role passed; 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) + } +} + +func TestDispatch_PrefetchesGoThroughApplyHook(t *testing.T) { + // Codex review #345 round 1: in-handler prefetches (members + // lookup for --assign, item.update's GET) used to bypass d.Apply, + // which would have meant any OAuth scope-context attached at + // dispatch time wouldn't apply to those side-channel reads — + // possible scope bypass when the future TASK-953 middleware + // wires Apply to attach token-allow-list context. + // + // This test asserts EVERY synthesized request (preprocess + // members lookup + item.update prefetch + main PATCH) flows + // through the Apply callback. + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/members", membersHandler(t, []memberRow{ + {UserID: "u-dave", UserName: "Dave"}, + }).ServeHTTP) + 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: + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + } + }) + + const marker = "X-Test-Apply-Marker" + applyCalls := []string{} + d := &HTTPHandlerDispatcher{ + Handler: mux, + UserResolver: fixedUserResolver(&models.User{ID: "caller"}), + Apply: func(r *http.Request) *http.Request { + applyCalls = append(applyCalls, r.Method+" "+r.URL.Path) + r.Header.Set(marker, "yes") + return r + }, + } + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", "ref": "TASK-5", + "status": "in-progress", + "assign": "Dave", + }) + 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) + } + + // Three requests synthesized: members lookup (preprocess) + + // item GET (prefetch) + item PATCH (main). All must have flowed + // through Apply. + wantPaths := map[string]bool{ + "GET /api/v1/workspaces/docapp/members": false, + "GET /api/v1/workspaces/docapp/items/TASK-5": false, + "PATCH /api/v1/workspaces/docapp/items/TASK-5": false, + } + for _, call := range applyCalls { + if _, expected := wantPaths[call]; expected { + wantPaths[call] = true + } + } + for path, seen := range wantPaths { + if !seen { + t.Errorf("Apply did not see %q (saw %v)", path, applyCalls) + } + } +} + +func TestRouteTable_ContainsWorkspaceMembers(t *testing.T) { + if _, ok := routeTable["workspace members"]; !ok { + t.Errorf("routeTable should include `workspace members` after TASK-967") + } + m, p, _, err := routeTable["workspace members"](map[string]any{"workspace": "docapp"}) + if err != nil { + t.Fatalf("err: %v", err) + } + if m != http.MethodGet { + t.Errorf("method = %q", m) + } + if p != "/api/v1/workspaces/docapp/members" { + t.Errorf("path = %q", p) + } +} + +// --- Test fixtures --- + +// memberRow is the test-side mirror of the (subset of) fields the +// resolveAssignName lookup reads from the workspace-members response. +type memberRow struct { + UserID string `json:"user_id"` + UserName string `json:"user_name"` + UserEmail string `json:"user_email"` +} + +// membersHandler returns an http.Handler that responds to any path +// with the standard `{members:[...], invitations:[]}` shape using +// the supplied rows. +func membersHandler(t *testing.T, rows []memberRow) http.Handler { + t.Helper() + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + body := map[string]any{ + "members": rows, + "invitations": []any{}, + } + _ = json.NewEncoder(w).Encode(body) + }) +} + +// errorHandler fails the test if invoked. Used to assert that a +// code path doesn't hit the handler at all. +func errorHandler(t *testing.T, msg string) http.Handler { + t.Helper() + return http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + t.Errorf("unexpected handler call: %s", msg) + }) +} + +// requestCapture is a tiny helper that records the last request + +// counts how many times it was called. Compatible with http.Handler +// so it slots into mux.Handle / mux.HandleFunc directly. +type requestCapture struct { + requestCount int + lastMethod string + lastPath string + lastBody string +} + +func newRequestCapture() *requestCapture { return &requestCapture{} } + +func (c *requestCapture) ServeHTTP(w http.ResponseWriter, r *http.Request) { + c.requestCount++ + c.lastMethod = r.Method + c.lastPath = r.URL.Path + if r.Body != nil { + body, _ := io.ReadAll(r.Body) + c.lastBody = string(body) + } + if w.Header().Get("Content-Type") == "" { + w.Header().Set("Content-Type", "application/json") + } + w.WriteHeader(http.StatusOK) + if r.Method == http.MethodPost { + _, _ = w.Write([]byte(`{"ok":true}`)) + return + } + _, _ = w.Write([]byte(`{}`)) +} diff --git a/internal/mcp/dispatch_http_routes.go b/internal/mcp/dispatch_http_routes.go index 6067b72f..1cb60253 100644 --- a/internal/mcp/dispatch_http_routes.go +++ b/internal/mcp/dispatch_http_routes.go @@ -229,6 +229,10 @@ func init() { method: http.MethodGet, pathTemplate: "/api/v1/workspaces/{workspace}/agent-roles", }.toRouteMapper(), + "workspace members": routeSpec{ + method: http.MethodGet, + pathTemplate: "/api/v1/workspaces/{workspace}/members", + }.toRouteMapper(), } } @@ -278,18 +282,12 @@ func mapItemList(input map[string]any) (string, string, []byte, error) { return "", "", nil, fmt.Errorf("workspace is required") } - if v, ok := input["assign"]; ok { - if s, ok := v.(string); ok && s != "" { - return "", "", nil, fmt.Errorf( - "--assign %q is not yet supported by HTTPHandlerDispatcher; "+ - "the CLI resolves names → user IDs via workspace-members "+ - "lookup, which we'll add in a follow-up. For now, pass "+ - "`--field assigned_user_id=` for explicit-ID filtering.", - s, - ) - } - } - + // `--assign` is preprocessed at the dispatcher level (TASK-967) — + // by the time we get here the name has been resolved to + // `assigned_user_id`. Old test fixtures that pass an explicit + // `assign` key directly to the mapper (bypassing Dispatch) just + // see the value silently dropped, matching the existing + // "unknown input keys are ignored" behaviour. pathBase := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/items" if coll, _ := input["collection"].(string); coll != "" { pathBase = "/api/v1/workspaces/" + url.PathEscape(workspace) + @@ -333,6 +331,13 @@ func mapItemList(input map[string]any) (string, string, []byte, error) { if s, _ := input["role"].(string); s != "" { add("agent_role_id", s) } + // assigned_user_id arrives here only via Dispatch's preprocess + // (which resolves --assign name → UUID via workspace.members). + // Agents that pass `--field assigned_user_id=` get the + // same treatment via the field-filter path further down. + if s, _ := input["assigned_user_id"].(string); s != "" { + add("assigned_user_id", s) + } // Numeric filters. if n, ok := numericInput(input["limit"]); ok && n > 0 { diff --git a/internal/mcp/dispatch_http_routes_test.go b/internal/mcp/dispatch_http_routes_test.go index 7cd878af..cd10ecb4 100644 --- a/internal/mcp/dispatch_http_routes_test.go +++ b/internal/mcp/dispatch_http_routes_test.go @@ -356,21 +356,44 @@ func TestRoute_ItemList_FiltersAsQuery(t *testing.T) { } } -func TestRoute_ItemList_RejectsAssignByName(t *testing.T) { - // CLI parity: --assign Dave resolves name→UUID via workspace - // members lookup. Replicating that prefetch in the dispatcher - // belongs in the same follow-up that handles --assign on - // item.create / update. For now, reject loudly so agents don't - // silently get empty results (Codex review #344 finding 3). - _, _, _, err := routeTable["item list"](map[string]any{ +func TestRoute_ItemList_PassesThroughAssignedUserID(t *testing.T) { + // TASK-967: --assign is preprocessed at the dispatcher level + // (resolveAssignName) before the mapper runs. By the time + // mapItemList sees the input, only `assigned_user_id` should be + // present; the mapper adds it to the query string for the + // store filter to pick up. + _, p, _, err := routeTable["item list"](map[string]any{ + "workspace": "docapp", + "assigned_user_id": "user-uuid-456", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + values := mustParseQueryFromPath(t, p) + if values.Get("assigned_user_id") != "user-uuid-456" { + t.Errorf("assigned_user_id missing from query: %v (path: %q)", values, p) + } +} + +func TestRoute_ItemList_RawAssignKeyIsIgnoredByMapper(t *testing.T) { + // Belt-and-braces: if a test or upstream caller bypasses + // Dispatch's preprocess and passes raw `assign` directly to the + // mapper, it should be silently dropped (matching the existing + // "unknown input keys are ignored" behaviour) rather than + // erroring. The dispatcher-level preprocess is what runs the + // resolution in production. + _, p, _, err := routeTable["item list"](map[string]any{ "workspace": "docapp", "assign": "Dave", }) - if err == nil { - t.Errorf("expected error for --assign by name; got nil") - return + if err != nil { + t.Fatalf("expected raw assign to pass through silently; got err: %v", err) } - if !strings.Contains(err.Error(), "assigned_user_id") { - t.Errorf("error should point users at the explicit-ID alternative; got %v", err) + values := mustParseQueryFromPath(t, p) + if values.Has("assigned_user_id") { + t.Errorf("raw assign incorrectly synthesized assigned_user_id: %v", values) + } + if values.Has("assign") { + t.Errorf("raw assign leaked into query: %v", values) } } diff --git a/internal/mcp/dispatch_http_test.go b/internal/mcp/dispatch_http_test.go index 2bc8ca16..9beca565 100644 --- a/internal/mcp/dispatch_http_test.go +++ b/internal/mcp/dispatch_http_test.go @@ -227,21 +227,158 @@ func TestMapItemCreate_NormalizesCollectionAliases(t *testing.T) { } } -func TestMapItemCreate_RejectsUnsupportedAssignRole(t *testing.T) { - for _, key := range []string{"assign", "role"} { - t.Run(key, func(t *testing.T) { - _, _, _, err := mapItemCreate(map[string]any{ - "workspace": "ws", "collection": "tasks", "title": "x", - key: "Dave", - }) - if err == nil { - t.Errorf("expected error rejecting --%s; got nil", key) - return - } - if !strings.Contains(err.Error(), key) { - t.Errorf("error should mention --%s; got %v", key, err) - } - }) +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{ + "workspace": "ws", "collection": "tasks", "title": "x", + "role": "implementer", + }) + if err == nil { + t.Errorf("expected error rejecting --role; got nil") + return + } + if !strings.Contains(err.Error(), "role") { + t.Errorf("error should mention --role; got %v", err) + } +} + +func TestMapItemCreate_LiftsAgentRoleIDFromFieldKVPToColumn(t *testing.T) { + // Codex review #345 round 3: the MCP tool schema only exposes + // `--role` (and the `--field` escape hatch), not a top-level + // `agent_role_id`. The error message tells agents to use + // `--field agent_role_id=`; the lift logic below makes + // that workaround reachable by recognizing column keys in the + // fields blob and moving them to the top-level payload before + // PATCH/POST. + _, _, body, err := mapItemCreate(map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "field": []any{"agent_role_id=role-uuid-789", "effort=l"}, + }) + if err != nil { + t.Fatalf("mapItemCreate: %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-789" { + t.Errorf("agent_role_id not lifted to top level; got %v", payload) + } + // And not in the fields blob. + fields := map[string]any{} + if s, _ := payload["fields"].(string); s != "" { + _ = json.Unmarshal([]byte(s), &fields) + } + if _, present := fields["agent_role_id"]; present { + t.Errorf("agent_role_id should be lifted out of fields blob: %v", fields) + } + // Other --field entries (effort=l) stay in the blob. + if fields["effort"] != "l" { + t.Errorf("non-column --field key should remain in fields blob: %v", fields) + } +} + +func TestMapItemCreate_LiftsAssignedUserIDFromFieldKVP(t *testing.T) { + // Companion to the agent_role_id lift — assigned_user_id is the + // other column key columnFieldKeys recognizes. + _, _, body, err := mapItemCreate(map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "field": []any{"assigned_user_id=user-uuid-12"}, + }) + if err != nil { + t.Fatalf("mapItemCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + if payload["assigned_user_id"] != "user-uuid-12" { + t.Errorf("assigned_user_id not lifted: %v", payload) + } +} + +func TestMapItemCreate_TopLevelAgentRoleIDWinsOverFieldKVP(t *testing.T) { + // Belt-and-braces: if both a top-level agent_role_id and a + // --field agent_role_id are set, the top-level wins. Avoids + // surprising callers who mix paths. + _, _, body, err := mapItemCreate(map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "agent_role_id": "explicit-uuid", + "field": []any{"agent_role_id=lift-uuid"}, + }) + if err != nil { + t.Fatalf("mapItemCreate: %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"] != "explicit-uuid" { + t.Errorf("explicit top-level agent_role_id should win; got %v", payload["agent_role_id"]) + } + // Lift still removes the duplicate from fields so the value + // doesn't appear in two places. + fields := map[string]any{} + if s, _ := payload["fields"].(string); s != "" { + _ = json.Unmarshal([]byte(s), &fields) + } + if _, present := fields["agent_role_id"]; present { + t.Errorf("agent_role_id should be removed from fields blob even when ignored: %v", fields) + } +} + +func TestMapItemCreate_PassesThroughAgentRoleID(t *testing.T) { + // agent_role_id (UUID) is the ItemCreate column the handler + // writes to. Agents that know the UUID (e.g. from a prior + // `role list` call) can set it without --role slug resolution. + // Codex review #345 round 2 caught the misleading error message + // pointing at `--field agent_role_id=` — that goes into + // the fields JSON blob, NOT the column. The fix is to pass the + // top-level `agent_role_id` through directly; this test pins + // that path so the workaround the error message points at + // actually works. + _, _, body, err := mapItemCreate(map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "agent_role_id": "role-uuid-789", + }) + if err != nil { + t.Fatalf("mapItemCreate: %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-789" { + t.Errorf("agent_role_id not passed through: %v", payload) + } + // And specifically not in the fields blob (where the misleading + // `--field agent_role_id=...` workaround would have put it). + if fields, _ := payload["fields"].(string); fields != "" { + t.Errorf("agent_role_id leaked into fields blob: %v", fields) + } +} + +func TestMapItemCreate_PassesThroughResolvedAssignedUserID(t *testing.T) { + // TASK-967: --assign is preprocessed at the dispatcher level + // (resolveAssignName) before the mapper runs. By the time + // mapItemCreate sees the input, only `assigned_user_id` should + // be present; the mapper passes it through to the body. + _, _, body, err := mapItemCreate(map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "assigned_user_id": "user-uuid-123", + }) + if err != nil { + t.Fatalf("mapItemCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + if payload["assigned_user_id"] != "user-uuid-123" { + t.Errorf("assigned_user_id not passed through: %v", payload) } }