mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 02:23:46 +00:00
xarmian
cb526a5875
* feat(mcp): item.update with field-merge + --assign name resolution + workspace.members (TASK-967, partial) Closes the biggest item-write gap in HTTPHandlerDispatcher: - `item.update` now works through the dispatcher with full CLI parity, including the read-modify-write merge of the item's fields JSON. Without RMW, `item update TASK-5 --status done` would erase every other field set on the item — Codex caught the equivalent top-level-vs-fields shape regression on item.create in PR #343. - `--assign <name|email>` now resolves to a user UUID via the workspace-members endpoint, matching the CLI's behaviour. Previously rejected with a clear error pointing at `--field assigned_user_id=<uuid>`; now the dispatcher does the lookup so agents can use the same human-friendly form they'd use in the CLI. - `workspace.members` is wired into the route table both as a tool (agents can list members directly) and as the resolution backend. ## Architecture - New `dispatch_http_advanced.go` holds dispatcher methods that need access to the wrapped `Handler` (for in-handler prefetches like RMW or member lookup). Stays separate from `dispatch_http_routes.go` which is the simple-mappers domain. - `Dispatch` now has a preprocess step that runs before the route table lookup: for an allowlisted set of commands (item.create / update / list), `--assign` is resolved to `assigned_user_id`. Other commands' input passes through untouched, so `item.show` won't spuriously hit the members endpoint. - `item.update` is a special case in Dispatch's switch — it doesn't use the route table because it needs to issue a GET-then-PATCH pair with merged fields. Other RMW commands (none for now) would slot into the same switch. - New `executeRequest` helper on the dispatcher centralizes the request-build → recorder → packageHTTPResponse path so the simple table-driven case and the RMW case share it without duplication. ## Tests - Unit: 7 tests on `resolveAssignName` covering pass-through (missing / empty), name + email matches (case-insensitive), no-match error, explicit-ID precedence, and members-endpoint failure propagation. - Unit: 3 Dispatch-level tests for the preprocess (success path, failure surfaces as IsError without dispatching the main request, allowlist scoping). - Unit: 4 dispatchItemUpdate tests covering the field merge, the no-fields-changes guard, prefetch 404 → no PATCH, and missing workspace/ref validation. - Integration: end-to-end through real *server.Server + SQLite — create with --assign Alice, then update --status without re-specifying priority / category, and assert the existing fields survive the merge. ## Out of scope - `--role <slug>` resolution. The route table has `role list` so agents can fetch the slug → ID mapping themselves; full prefetch-resolution lives in the next route-table expansion. mapItemCreate now rejects `--role` loudly (was previously bundled with the `--assign` rejection). - The remaining ~50 commands (links, dependencies, star, project intelligence, library, github, attachments, webhooks, …) — TASK-967 stays open for follow-up PRs. Parent: PLAN-943. * fix(mcp): item.update --role rejection + Apply hook on prefetches per Codex review (round 1) Codex caught two real parity / security issues: 1. `item.update` silently ignored `--role`. mapItemCreate rejects it loudly because slug → role-ID resolution isn't built yet, but dispatchItemUpdate was a separate code path that didn't echo the guard. Agents would have gotten a successful update response while the role assignment got dropped on the floor. Reject with the same message + same pointer to `--field agent_role_id=<uuid>`. 2. The new prefetches (workspace.members lookup for --assign, item.update GET) bypassed `d.Apply`. Apply is the OAuth-scope hook the future TASK-953 middleware will use to attach token-allow-list / capability- tier context. Prefetches running outside that hook would have been a scope-bypass surface — agents could read members or items their token shouldn't have access to during resolution. Centralized the build + apply step in a new buildAuthedRequest helper on HTTPHandlerDispatcher. Both the in-handler prefetches and executeRequest now go through it so every synthesized request — main PATCH, GET prefetch, members lookup — sees Apply uniformly. Tests: - TestDispatchItemUpdate_RejectsUnsupportedRole asserts the role rejection trips before any handler call. - TestDispatch_PrefetchesGoThroughApplyHook asserts all three request types in an --assign + RMW flow (members lookup, item GET, item PATCH) flow through the Apply callback. Parent: PLAN-943. * fix(mcp): real --role workaround pass-through + corrected error message per Codex review (round 2) Codex caught a misleading error message: the rejection of --role pointed agents at `--field agent_role_id=<uuid>` as the workaround, but `--field` writes into the item's fields JSON blob, NOT the agent_role_id column on the ItemCreate / ItemUpdate models. So agents following the workaround would have ended up with the value sitting inert inside fields and the actual role assignment unchanged — the exact silent-success failure mode the rejection was meant to prevent. Two changes: 1. mapItemCreate + dispatchItemUpdate now pass `agent_role_id` through to the request payload at the top level, the same way `assigned_user_id` is passed through. So an agent that knows the role's UUID (from a prior `role list` call) can set it without --role slug resolution. 2. The error message text now points at "pass `agent_role_id=<uuid>` directly in the tool input" instead of `--field agent_role_id=...`. Adds the hint to discover the UUID via `role list` since that's the workflow agents would follow. Tests: - TestMapItemCreate_PassesThroughAgentRoleID asserts agent_role_id lands at the top level of the create body, NOT inside fields. - TestDispatchItemUpdate_PassesThroughAgentRoleID asserts the same for the update PATCH body. Parent: PLAN-943. * fix(mcp): lift column keys out of --field blob so the --role workaround is reachable per Codex review (round 3) Codex caught: the rejection of --role pointed agents at `agent_role_id=<uuid>` as a top-level input, but the MCP tool schema (auto-generated from cmdhelp) doesn't expose a top-level `agent_role_id` flag — only `--role` and `--field`. Strict clients (Claude Desktop, Cursor) following the schema have no way to send the recommended workaround as written. Two changes: 1. Added a `liftFieldsToColumns` helper that scans the fields blob for column keys (agent_role_id, assigned_user_id) and moves them onto the top-level payload. mapItemCreate runs this before serializing fields; dispatchItemUpdate runs it after the merge step. 2. The error message text now points at `--field agent_role_id=<uuid>` — that IS in the schema, and the lift logic ensures the value actually reaches the column rather than sitting inert in the fields JSON. The text explains the lift behaviour explicitly so readers don't think it's a hack. This makes the workaround genuinely reachable for any agent that follows the auto-generated tool schema. The "right" long-term fix (adding agent_role_id and assigned_user_id as proper named flags in cmdhelp so the schema exposes them directly) is captured for TASK-968. Tests: - TestMapItemCreate_LiftsAgentRoleIDFromFieldKVPToColumn pins the lift end-to-end on item.create. - TestMapItemCreate_LiftsAssignedUserIDFromFieldKVP confirms the same path works for the other column key. - TestMapItemCreate_TopLevelAgentRoleIDWinsOverFieldKVP locks the precedence rule (top-level wins; lift still strips the dup). - TestDispatchItemUpdate_LiftsAgentRoleIDFromFieldKVPToColumn pins the same behaviour on item.update's PATCH path. Parent: PLAN-943.