From 853816efaa656c5cb31aac6f9fcba449a358903a Mon Sep 17 00:00:00 2001 From: Tim Froehlich Date: Thu, 17 Sep 2026 08:37:42 -0500 Subject: [PATCH 1/4] feat(mcp): reduce MCP server context overhead and consolidate update_machine (PP-u4ab.18) - Compact tool descriptions into concise 1-2 sentence functional contracts (Tactic 1) - Offload procedural runbooks (offset draining, PinballMap/iScored linking) to .agents/skills/pinpoint-mcp/SKILL.md - Implement update_machine consolidating granular machine mutation tools (Tactic 3) - Register update_machine in tool catalog and update integration tests - Track divergence row in docs/feature-specs/iscored.md --- .agents/skills/pinpoint-mcp/SKILL.md | 96 ++++ .agents/skills/pinpoint-pinballmap/SKILL.md | 7 + docs/feature-specs/iscored.md | 1 + src/app/api/mcp/mcp/route.ts | 4 +- src/lib/mcp/tools/add-issue-comment.ts | 2 +- src/lib/mcp/tools/create-issue.ts | 2 +- src/lib/mcp/tools/get-issue.ts | 2 +- src/lib/mcp/tools/get-machine.ts | 2 +- src/lib/mcp/tools/index.ts | 19 +- src/lib/mcp/tools/list-issues.ts | 19 +- src/lib/mcp/tools/list-machines.ts | 27 +- .../mcp/tools/search-pinballmap-catalog.ts | 2 +- src/lib/mcp/tools/update-issue.ts | 2 +- src/lib/mcp/tools/update-machine.ts | 377 ++++++++++++++ src/test/integration/mcp-tools.test.ts | 461 +++++++++++++++++- 15 files changed, 960 insertions(+), 63 deletions(-) create mode 100644 .agents/skills/pinpoint-mcp/SKILL.md create mode 100644 src/lib/mcp/tools/update-machine.ts diff --git a/.agents/skills/pinpoint-mcp/SKILL.md b/.agents/skills/pinpoint-mcp/SKILL.md new file mode 100644 index 000000000..119d6d027 --- /dev/null +++ b/.agents/skills/pinpoint-mcp/SKILL.md @@ -0,0 +1,96 @@ +--- +name: pinpoint-mcp +description: Operational runbook and conventions for interacting with PinPoint via the MCP server. Use when querying or modifying PinPoint records (machines, issues, comments, PinballMap links, iScored IDs) through MCP tools, performing fleet status sweeps, or updating machines. Covers the pagination drain procedure, PinballMap linking flow, iScored game ID management, and safety/confirmation rules. +--- + +# PinPoint MCP Operations Guide + +This skill governs interaction with PinPoint's Model Context Protocol (MCP) server for remote administration of the Austin Pinball Collective (APC) collection and issue tracker. + +Every MCP call executes within Tim's admin identity (`accessLevel: "admin"`). Writes are audit-logged and attributed to Tim across timelines and notifications. + +--- + +## 1. Tool Catalog & Target Conventions + +### Target Identification + +- **Machines**: Identified by `machine` parameter, which accepts **initials** (case-insensitive, e.g. `"MM"`, `"AFM"`, `"TZ"`) or machine UUID. Initials are the primary human-friendly key. +- **Issues**: Identified by **machine + issue number** (e.g. `machine: "MM"`, `number: 3`), mirroring the app URL `/m//i/`. + +### Summary of Tools + +- `whoami`: Returns resolved identity, access level, client ID, and auth mode. +- `list_machines`: Lists cabinets with initials, name, availability, owner, and open issue counts. +- `get_machine`: Full machine detail including PinballMap link, iScored link, and open issues. +- `add_machine`: Create a new machine row. +- `update_machine`: Consolidated tool to update machine `name`, `presenceStatus`, `owner`, PinballMap link (`pinballmapMachineId` / `pinballmapExcluded`), lineup intent (`intent`), or `iscoredGameId`. +- `list_issues`: Lists issues across the collection or for a single machine with filters. +- `get_issue`: Full issue detail including plain-text description, assignee, reporter, and comment thread. +- `create_issue`: Files a new issue on a machine. +- `add_issue_comment`: Adds a comment to an issue thread. +- `update_issue`: Updates one or more issue fields (`title`, `status`, `severity`, `priority`, `frequency`, `assignee`). +- `search_pinballmap_catalog`: Two-step lookup in PinPoint's local PinballMap catalog mirror. + +--- + +## 2. Paging & Mutating Worklists (The Drain Procedure) + +When performing batch triage or sweeping a worklist (e.g., "put all off-the-floor machines on the floor", "triage all new issues", or "link unlinked cabinets"): + +> [!WARNING] +> **Do NOT advance `offset += limit` when your operations mutate the rows you are filtering by.** + +### The Offset Shifting Trap + +When you query a filtered list (such as `list_machines(presence: "off_the_floor", offset: 0, limit: 10)`), and then update those 10 machines to `presenceStatus: "on_the_floor"`: + +1. Those 10 machines immediately leave the `off_the_floor` filter. +2. All remaining matches shift up to fill the vacated positions. +3. If you subsequently request `offset: 10`, you skip over the 10 machines that just shifted into indices 0–9! + +### The Canonical Drain Pattern + +1. Always query with `offset: 0`. +2. Inspect and update the returned page. +3. Because the updated items leave the filter, re-query with `offset: 0` and let the list drain. +4. If there are specific rows you deliberately choose **not** to change, advance `offset` past _only_ those unchanged rows so they do not repeat. +5. The sweep is complete when a request returns an empty page (`count: 0`), **not** when `total: 0` (since unchanged rows hold `total` above 0). + +--- + +## 3. PinballMap Machine Linking Procedure + +PinPoint maintains a local mirror of the PinballMap catalog. Linking a machine requires a two-step lookup: + +1. **Search Families**: Call `search_pinballmap_catalog(query: "title name")`. + - This returns edition families or standalone titles with an `editionCount`. + - If `machineGroupId` is `null` or `editionCount` is `1`, the game is standalone and its `pinballmapMachineId` is already returned. That is your answer. +2. **List Editions**: If `machineGroupId` is non-null and `editionCount > 1`: + - Call `search_pinballmap_catalog(machineGroupId: )` to retrieve individual editions (Pro, Premium, LE, etc.). + - Verify `familyName` on the response to confirm you passed a family group ID, not an edition ID. + - Select the edition's `pinballmapMachineId`. +3. **Link via `update_machine`**: + - Call `update_machine(machine: "", pinballmapMachineId: , intent: "on" | "off" | "no_sync")`. +4. **Excluding Uncataloged Cabinets**: + - For homebrew or uncataloged one-offs: `update_machine(machine: "", pinballmapExcluded: true, pinballmapExcludedReason: "Custom homebrew cabinet")`. + - `pinballmapMachineId` and `pinballmapExcluded` are mutually exclusive. + +--- + +## 4. iScored High Scores Linking + +PinPoint integrates with iScored for arcade leaderboard displays: + +- **Link a game**: `update_machine(machine: "", iscoredGameId: "")`. +- **Clear a link**: `update_machine(machine: "", iscoredGameId: null)` or pass an empty string. + +--- + +## 5. Safety, Audit & Operator Confirmation + +- **Inspect First**: Always inspect the target with `get_machine` or `get_issue` before performing an update. +- **Reversible vs Irreversible Actions**: + - Reversible: updating presence, owner, or name. + - Public/Notifying: `create_issue` and comments dispatch notifications to machine owners and watchers. +- **Explicit Confirmation**: Obtain user confirmation before executing batch sweeps or filing issues on someone else's behalf. diff --git a/.agents/skills/pinpoint-pinballmap/SKILL.md b/.agents/skills/pinpoint-pinballmap/SKILL.md index 1d51fec53..0642f90bb 100644 --- a/.agents/skills/pinpoint-pinballmap/SKILL.md +++ b/.agents/skills/pinpoint-pinballmap/SKILL.md @@ -34,3 +34,10 @@ Preserve the live client's serialized writes and bounded `429` handling. Reuse s When showing data for a specific PinballMap location, use `pinballmapLocationUrl` for the required location-listing attribution; do not construct the URL or link only to the homepage. Unit and E2E tests must use the mock client at the seam and committed captured fixtures. The fixture-refresh script is a deliberate manual GET-only operation, never test setup or a routine live call. + +## MCP Catalog Linking & Lineup Intent + +When managing PinballMap links via the PinPoint MCP server: + +- Use `search_pinballmap_catalog` (2-step family → edition lookup) and `update_machine(machine, pinballmapMachineId: ..., intent: "on" | "off" | "no_sync")`. +- For the full 2-step procedure and mutual exclusion rules (`pinballmapExcluded`), see the `pinpoint-mcp` skill (`.agents/skills/pinpoint-mcp/SKILL.md`). diff --git a/docs/feature-specs/iscored.md b/docs/feature-specs/iscored.md index 03f68653e..b715d6bcf 100644 --- a/docs/feature-specs/iscored.md +++ b/docs/feature-specs/iscored.md @@ -64,6 +64,7 @@ Removed 2026-09-16. The Info tab card's "View all on iScored" link replaces it: | Spec | Code today | Resolution | | :-- | :-- | :-- | | §2.1–§2.2 machine linking in Manage tab | No form field in Manage tab | PP-h2bu.4 | +| §2.3 MCP tool | Handled via consolidated `update_machine` tool | PP-u4ab.18 | | §4.1–§4.5 Info tab top scores card | Not yet rendered | PP-h2bu.4 | | §6.1 Fleet overview column | Not yet rendered | PP-h2bu.5 | diff --git a/src/app/api/mcp/mcp/route.ts b/src/app/api/mcp/mcp/route.ts index b68098ff1..b010286e3 100644 --- a/src/app/api/mcp/mcp/route.ts +++ b/src/app/api/mcp/mcp/route.ts @@ -22,9 +22,7 @@ export const maxDuration = 60; * each tool additionally runs `checkPermission()` underneath (defense in depth). * * Tools: the PinPoint tool catalog ({@link registerPinpointTools}) plus a - * `whoami` diagnostic used to validate the connection end-to-end. Deliberately - * no count here — that number goes stale every time a tool lands (PP-x8jb); - * `registerPinpointTools` is the list. + * `whoami` diagnostic used to validate the connection end-to-end. */ const handler = createMcpHandler( (server) => { diff --git a/src/lib/mcp/tools/add-issue-comment.ts b/src/lib/mcp/tools/add-issue-comment.ts index f0e5a733a..c9c2c5e9e 100644 --- a/src/lib/mcp/tools/add-issue-comment.ts +++ b/src/lib/mcp/tools/add-issue-comment.ts @@ -108,7 +108,7 @@ export function registerAddIssueComment(server: McpServer): void { { title: "Comment on an issue", description: - "Post a comment on an issue, attributed to the authenticated user. Identify the issue by machine (initials or UUID) plus the issue number shown in its URL and returned by list_issues, get_machine, and create_issue. Plain text only — markdown is not rendered. Retrying an identical comment shortly after one usually resolves to the comment already posted instead of a duplicate — check 'created' in the response: false means nothing new was written, so report it as already posted rather than as a new comment.", + "Add a comment to an existing issue. Requires machine (initials or UUID), issue number, and comment text.", inputSchema: addIssueCommentSchema, annotations: WRITE_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/create-issue.ts b/src/lib/mcp/tools/create-issue.ts index b189c405a..cd43ec632 100644 --- a/src/lib/mcp/tools/create-issue.ts +++ b/src/lib/mcp/tools/create-issue.ts @@ -148,7 +148,7 @@ export function registerCreateIssue(server: McpServer): void { { title: "Create issue", description: - "File an issue against a machine (identified by initials or UUID). Requires a title; optional plain-text description, severity (cosmetic/minor/major/unplayable), priority (low/medium/high), and frequency (intermittent/frequent/constant). Attributed to the authenticated admin. Retrying an identical call shortly after one usually resolves to the issue already filed instead of a duplicate — check 'created' in the response: false means nothing new was written and 'number' refers to the pre-existing issue, so report it as already filed rather than as a new one.", + "File a new issue on a machine. Requires machine (initials or UUID), title, and severity; accepts optional description, priority, and frequency.", inputSchema: createIssueSchema, annotations: WRITE_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/get-issue.ts b/src/lib/mcp/tools/get-issue.ts index ef4822dff..e9536cbc6 100644 --- a/src/lib/mcp/tools/get-issue.ts +++ b/src/lib/mcp/tools/get-issue.ts @@ -176,7 +176,7 @@ export function registerGetIssue(server: McpServer): void { { title: "Get issue detail", description: - "Get one issue in full — title, description, status, severity, priority, frequency, reporter and assignee names, timestamps, URL, and the comment thread. Identify it by machine (initials or UUID) plus the issue number shown in its URL and returned by list_issues, get_machine, and create_issue. Use this before commenting or updating, so you are acting on the issue you think you are. The thread returns the MOST RECENT comments (20 by default, up to 100 via commentLimit), listed oldest-first within that window; 'commentCount' is the full thread length and 'commentsTruncated' is true when older comments were left out, so raise commentLimit if you need the earlier history. Timeline/system rows are not included in the thread; the issue's current status is what they would describe.", + "Get full details for an issue by machine (initials or UUID) and issue number: title, description, status, severity, priority, frequency, reporter/assignee names, timestamps, and recent comment thread.", inputSchema: getIssueSchema, annotations: READ_ONLY_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/get-machine.ts b/src/lib/mcp/tools/get-machine.ts index a554d2129..f79f11c7c 100644 --- a/src/lib/mcp/tools/get-machine.ts +++ b/src/lib/mcp/tools/get-machine.ts @@ -98,7 +98,7 @@ export function registerGetMachine(server: McpServer): void { { title: "Get machine detail", description: - "Get one machine's detail — name, initials, availability (returned as `presence`), owner name, its iScored game ID (or null), its Pinball Map state (linked catalog title and edition, manufacturer, year, OPDB/IPDB, and the operator's lineup intent — `on`, `off`, or `no_sync` — which says whether it SHOULD be on the location's public lineup, not whether it currently is; or marked as not on Pinball Map; or null when neither has been recorded — and when a linked title is null, read `catalogLookup` before calling the link broken: `mirror_unpopulated` means PinPoint's catalog copy is empty, not that the link is stale), and its recent open issues (each with number, title, severity, status, and URL). Identify the machine by initials or UUID.", + "Get full details for a machine by initials or UUID: name, presence status, owner name, Pinball Map link state and lineup intent, and recent open issues.", inputSchema: getMachineSchema, annotations: READ_ONLY_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/index.ts b/src/lib/mcp/tools/index.ts index 102f333fa..968c24f55 100644 --- a/src/lib/mcp/tools/index.ts +++ b/src/lib/mcp/tools/index.ts @@ -10,22 +10,17 @@ import { registerGetMachine } from "./get-machine"; import { registerListIssues } from "./list-issues"; import { registerListMachines } from "./list-machines"; import { registerSearchPinballmapCatalog } from "./search-pinballmap-catalog"; -import { registerSetMachineAvailability } from "./set-machine-availability"; -import { registerSetMachineIscored } from "./set-machine-iscored"; -import { registerSetMachineName } from "./set-machine-name"; -import { registerSetMachineOwner } from "./set-machine-owner"; -import { registerSetMachinePinballmap } from "./set-machine-pinballmap"; import { registerUpdateIssue } from "./update-issue"; +import { registerUpdateMachine } from "./update-machine"; /** * Register the MCP tool catalog (spec §"Tool catalog") on an McpServer. Reads * for disambiguation plus mutations, every one admin-gated at the door and * `checkPermission`-gated per call. * - * Two entities, each covered end to end: machines (list, read, add, rename, - * set availability, set owner, set PinballMap title) and issues (list, read, - * file, comment, update), plus the PinballMap catalog lookup that identifies a - * machine's title. + * Two entities, each covered end to end: machines (list, read, add, update) + * and issues (list, read, file, comment, update), plus the PinballMap catalog + * lookup that identifies a machine's title. * * This function is the catalog — a tool that ships without a call here is * unreachable no matter how complete its handler is, which is what the @@ -37,12 +32,8 @@ export function registerPinpointTools(server: McpServer): void { registerListIssues(server); registerGetIssue(server); registerSearchPinballmapCatalog(server); - registerSetMachineAvailability(server); - registerSetMachineName(server); registerAddMachine(server); - registerSetMachineOwner(server); - registerSetMachinePinballmap(server); - registerSetMachineIscored(server); + registerUpdateMachine(server); registerCreateIssue(server); registerAddIssueComment(server); registerUpdateIssue(server); diff --git a/src/lib/mcp/tools/list-issues.ts b/src/lib/mcp/tools/list-issues.ts index 850946acc..66e185323 100644 --- a/src/lib/mcp/tools/list-issues.ts +++ b/src/lib/mcp/tools/list-issues.ts @@ -65,7 +65,7 @@ const listIssuesSchema = z.object({ status: statusFilterSchema .optional() .describe( - "Which statuses to include: 'open' (the default), 'closed', one status, or an array of statuses. Open statuses are new, confirmed, wait_owner, in_progress, need_parts, need_help. Closed are fixed, wont_fix, wai, no_repro, duplicate." + "Which statuses to include: 'open' (default), 'closed', a single status, or an array of statuses." ), severity: z .enum(ISSUE_SEVERITY_VALUES) @@ -93,9 +93,7 @@ const listIssuesSchema = z.object({ .int() .min(0) .optional() - .describe( - "How many matches to skip. Issues come back newest first, with machine initials and issue number breaking ties — a total order, so separate requests agree about where a page boundary falls, for as long as the underlying rows don't change. Whether you should advance this offset at all depends on whether your own calls change what matches; the tool description has the rule." - ), + .describe("Number of matches to skip for pagination."), }); type ListIssuesArgs = z.infer; @@ -192,18 +190,11 @@ export async function runListIssues( } /** - * Why the description repeats `list_machines`' drain procedure. + * Offset paging over a mutating result set. * * Offset paging is coherent only over a result set that holds still, and * `update_issue` writes every field this tool filters on — `status` (which is - * also the DEFAULT filter), `severity`, and `assignee`. Working a filtered - * worklist while paging it is the normal use here rather than an edge case, so - * the failure sits on the common path: each issue actioned leaves the filter, - * the rest shift up, and `offset += limit` steps over exactly the ones that - * moved. - * - * Stated once in the description, for the model that has to follow it; this - * comment is the rationale, not a second copy. + * also the DEFAULT filter), `severity`, and `assignee`. */ export function registerListIssues(server: McpServer): void { server.registerTool( @@ -211,7 +202,7 @@ export function registerListIssues(server: McpServer): void { { title: "List issues", description: - "Find issues across the whole collection, or on one machine. Every row carries the machine initials and issue number you need to act on it with get_issue, add_issue_comment, or update_issue. Filters: machine, status ('open' by default, or 'closed', or a specific set like ['need_parts','need_help']), severity, and assignee. Returns 'count' (this page), 'total' (every match), 'offset', and 'hasMore'. Answer counting questions from 'total', never from 'count' or the array length. To enumerate more than one page, keep requesting with offset += limit until hasMore is false — raising limit alone caps at 100 and will not reach the rest. That works only while the matching set holds still, and your own calls move it: update_issue changes status, severity, and assignee, which are exactly the filters here. So if you are ACTING on the issues as you page them — 'triage every new issue', 'close everything already fixed' — do NOT advance the offset. Each issue you action leaves the filter and the rest shift up, so offset += limit steps over exactly as many issues as you just handled, and the sweep ends on hasMore:false having never shown them. Re-request offset 0 and let the list drain instead. Raise offset only past issues you deliberately left unchanged, so they don't keep coming back. You are done when a request returns EMPTY (count 0), NOT when total reaches 0 — issues you left unchanged hold total above 0 forever.", + "List issues across the entire collection or on a specific machine. Supports filtering by machine (initials/UUID), status ('open', 'closed', or specific statuses), severity, and assignee. Returns paginated results with total count and hasMore.", inputSchema: listIssuesSchema, annotations: READ_ONLY_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/list-machines.ts b/src/lib/mcp/tools/list-machines.ts index 4ed2e0628..04e2c9287 100644 --- a/src/lib/mcp/tools/list-machines.ts +++ b/src/lib/mcp/tools/list-machines.ts @@ -123,13 +123,13 @@ export const listMachinesSchema = z.object({ presence: presenceFilterSchema .optional() .describe( - "Which availability statuses to include: one status, or an array of them. The statuses are on_the_floor, off_the_floor, on_loan, pending_arrival, removed. A cabinet in any of the first three is still plausibly in the collection; removed is gone and pending_arrival has not arrived yet." + "Which availability statuses to include: a single status or an array of statuses (on_the_floor, off_the_floor, on_loan, pending_arrival, removed)." ), pinballmap: z .enum(PINBALLMAP_FILTERS) .optional() .describe( - "Only machines in this PinballMap link state. 'unlinked' = no catalog match yet and not marked as absent from PinballMap (the linking worklist); 'linked' = matched to a catalog title; 'excluded' = deliberately marked as not on PinballMap. Combines with 'search' and 'presence'." + "Filter by PinballMap link state: 'unlinked' (no catalog match and not excluded), 'linked' (matched to catalog), or 'excluded' (marked not on PinballMap)." ), limit: z .number() @@ -145,9 +145,7 @@ export const listMachinesSchema = z.object({ .int() .min(0) .optional() - .describe( - "How many matches to skip, for paging past the limit. Machines are ordered by name, then by initials to break ties between duplicate cabinets of the same title — a total order, so separate requests agree with each other about where a page boundary falls, for as long as the underlying rows don't change. Whether you should advance this offset at all depends on whether your own calls change what matches; the tool description has the rule." - ), + .describe("Number of matches to skip for pagination."), }); type ListMachinesArgs = z.infer; @@ -242,26 +240,13 @@ export async function runListMachines( } /** - * Why the description spends so many words on paging. + * Offset paging over a mutating result set. * * Offset paging is only coherent over a result set that holds still, and the MCP * surface can move it: `set_machine_availability` writes `presenceStatus` (the * `presence` filter), `set_machine_name` writes `name` (both the `search` target * and the primary sort key), and `add_machine` inserts rows that can land inside - * any filter. So "page a filter, act on each row" — the natural reading of "put - * every off-the-floor machine back on the floor" — silently skips about half of - * them: each machine acted on leaves the filter, the rest shift up, and the next - * `offset += limit` steps over exactly the ones that moved. - * - * That is why the description gives the drain procedure rather than just a - * warning. It is stated once, there, for the model that has to follow it; this - * comment is the rationale, not a second copy. - * - * `pinballmap` used to be the exception, by accident of what was not built yet. - * `set_machine_pinballmap` (PP-u4ab.12) ships the link verb, so link state is - * now mutable like the rest and the fleet linking pass moves rows out of the - * `unlinked` bucket as it goes — exactly the shape the drain procedure exists - * for. No edit to the description was needed: it was written to cover this. + * any filter. */ export function registerListMachines(server: McpServer): void { server.registerTool( @@ -269,7 +254,7 @@ export function registerListMachines(server: McpServer): void { { title: "List machines", description: - "List machines with their initials, name, availability, owner name, and open-issue count. Use this to find a machine's initials before acting on it (e.g. disambiguate 'the Medieval Madness by the door'). Supports a name/initials search, a presence filter (one status, or an array of them to accept several at once), and a PinballMap link-state filter (pinballmap: 'unlinked' | 'linked' | 'excluded') — use pinballmap: 'unlinked' to get the machines still needing a PinballMap catalog match. For that linking pass, ask for pinballmap: 'unlinked' TOGETHER WITH presence: ['on_the_floor', 'on_loan', 'off_the_floor'], which is the actionable worklist. 'unlinked' on its own also returns cabinets that are 'removed' (no longer in the collection) or 'pending_arrival' (not here yet) — nobody will ever link those, so they come back on every page of every sweep and you would have to recognise and skip them by hand each time. Narrowing presence drops them from 'total' as well as from the page, so 'total' is the size of the work actually left. Returns 'count' (this page), 'total' (every match), 'offset', and 'hasMore'. Answer counting questions from 'total', never from 'count' or the array length. To enumerate a collection larger than one page, keep requesting with offset += limit until hasMore is false — raising limit alone caps at 100 and will not reach the rest. That works only while the matching set holds still, and your own calls can move it: set_machine_availability changes presence, set_machine_name changes name (the search target and the sort key), add_machine adds rows. So if you are ACTING on the machines as you page them — 'put every off-the-floor machine back on the floor' — do NOT advance the offset. Each machine you fix leaves the filter and the rest shift up, so offset += limit steps over exactly as many machines as you just fixed, and the sweep ends on hasMore:false having never shown them. Re-request offset 0 and let the list drain instead. Raise offset only past machines you deliberately left unchanged, so they don't keep coming back. You are done when a request returns EMPTY (count 0), NOT when total reaches 0 — machines you left unchanged hold total above 0 forever. Narrowing the filter so it matches only rows you can actually act on, as the presence set above does, is what lets total fall to 0 at all.", + "List machines with initials, name, availability (presence), owner name, and open-issue count. Supports search by name/initials, presence filtering, and PinballMap link-state filtering ('unlinked' | 'linked' | 'excluded'). Returns paginated results with total count and hasMore.", inputSchema: listMachinesSchema, annotations: READ_ONLY_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/search-pinballmap-catalog.ts b/src/lib/mcp/tools/search-pinballmap-catalog.ts index 5ad5eba39..9f154cad0 100644 --- a/src/lib/mcp/tools/search-pinballmap-catalog.ts +++ b/src/lib/mcp/tools/search-pinballmap-catalog.ts @@ -229,7 +229,7 @@ export function registerSearchPinballmapCatalog(server: McpServer): void { { title: "Search the Pinball Map catalog", description: - "Find a Pinball Map catalog title to identify a machine's model/edition. Two steps, like the web picker: pass `query` to get matching families (an edition group such as 'Elvira's House of Horrors', or a standalone title) with an `editionCount`; then, ONLY for a family with a non-null `machineGroupId` and `editionCount` above 1, pass that `machineGroupId` to list its individual editions (Pro/Premium/LE) with the `pinballmapMachineId` of each. A standalone title — `machineGroupId` null, or `editionCount` 1 — already carries its `pinballmapMachineId`: that is the answer, don't call again. Most pre-1990s machines are standalone. Pass one argument or the other, never both. CHECK `familyName` ON THE EDITIONS RESPONSE before using any id from it: machine ids and machine-group ids are separate id spaces, so the wrong integer can return a real but unrelated family's editions. If `familyName` is not the title you searched for, you passed an edition's `pinballmapMachineId` instead of a family's `machineGroupId` — search by name again. `returned` is this page's size, not a total — this tool cannot answer 'how many titles are there'; when `hasMore` is true, narrow the query, as families come back best-match first and there is no paging. Read-only, served from PinPoint's local catalog mirror.", + "Search the local Pinball Map catalog mirror. Pass 'query' to search edition families and standalone titles; pass 'machineGroupId' to list individual editions for a multi-edition family. Returns matching titles with their pinballmapMachineId.", inputSchema: searchPinballmapCatalogSchema, annotations: READ_ONLY_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/update-issue.ts b/src/lib/mcp/tools/update-issue.ts index f0e9ae3bf..0008bc0ea 100644 --- a/src/lib/mcp/tools/update-issue.ts +++ b/src/lib/mcp/tools/update-issue.ts @@ -344,7 +344,7 @@ export function registerUpdateIssue(server: McpServer): void { { title: "Update an issue", description: - "Change one or more fields on an issue: title, status, severity, frequency, priority, or assignee. Identify the issue by machine (initials or UUID) plus the issue number shown in its URL. Supply only the fields you want to change; at least one is required. The response returns 'applied' — one entry per field with 'from', 'to', and 'changed', where changed:false means the issue already held that value and nothing was written. Fields are applied one at a time and are NOT a single transaction: if one fails, the fields before it stay written and the response comes back with 'partial': true plus 'failed' naming the field that stopped it. Read 'applied' rather than assuming the whole call landed.", + "Update one or more fields on an issue: title, status, severity, frequency, priority, or assignee. Supply machine (initials or UUID), issue number, and at least one field to change. Returns applied changes.", inputSchema: updateIssueSchema, annotations: WRITE_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/update-machine.ts b/src/lib/mcp/tools/update-machine.ts new file mode 100644 index 000000000..70539cdd1 --- /dev/null +++ b/src/lib/mcp/tools/update-machine.ts @@ -0,0 +1,377 @@ +import "server-only"; + +import type { McpServer } from "@modelcontextprotocol/server"; +import { after } from "next/server"; +import { z } from "zod"; + +import { dispatchNotification } from "~/lib/notifications"; +import { checkPermission } from "~/lib/permissions/helpers"; +import { VALID_MACHINE_PRESENCE_STATUSES } from "~/lib/machines/presence"; +import { + updateMachineIscoredLink, + updateMachineName, + updateMachineOwner, + updateMachinePbmLink, + updateMachinePresence, +} from "~/services/machines"; + +import { + getOwnerNamesByMachine, + machineUrl, + McpToolError, + resolveMachine, + resolveOwner, + runTool, + type ToolOutcome, + WRITE_TOOL_ANNOTATIONS, +} from "./shared"; +import type { McpAuthContext } from "~/lib/mcp/verify-token"; + +export const updateMachineSchema = z + .object({ + machine: z + .string() + .trim() + .min(1) + .describe("Machine initials (case-insensitive) or UUID."), + name: z + .string() + .trim() + .min(1) + .max(200) + .optional() + .describe("New machine name."), + presenceStatus: z + .enum(VALID_MACHINE_PRESENCE_STATUSES) + .optional() + .describe("New availability status."), + owner: z + .string() + .trim() + .nullable() + .optional() + .describe("New owner name or UUID, or empty string/null to clear."), + pinballmapMachineId: z + .number() + .int() + .positive() + .optional() + .describe("Pinball Map catalog machine ID."), + pinballmapExcluded: z + .literal(true) + .optional() + .describe("Mark machine as excluded from Pinball Map."), + pinballmapExcludedReason: z + .string() + .trim() + .min(1) + .max(200) + .optional() + .describe("Reason for Pinball Map exclusion."), + intent: z + .enum(["on", "off", "no_sync"]) + .optional() + .describe("Lineup sync intent for Pinball Map."), + iscoredGameId: z + .string() + .trim() + .nullable() + .optional() + .describe( + "iScored game ID string to link to this machine, or null/empty string to clear." + ), + }) + .refine( + (args) => + args.name !== undefined || + args.presenceStatus !== undefined || + args.owner !== undefined || + args.pinballmapMachineId !== undefined || + args.pinballmapExcluded !== undefined || + args.pinballmapExcludedReason !== undefined || + args.intent !== undefined || + args.iscoredGameId !== undefined, + { + message: + "Supply at least one field to change: name, presenceStatus, owner, pinballmapMachineId, pinballmapExcluded, pinballmapExcludedReason, intent, or iscoredGameId.", + } + ) + .refine( + (args) => + !( + args.pinballmapMachineId !== undefined && + args.pinballmapExcluded === true + ), + { + message: + "A machine can't be both linked to a Pinball Map title and marked as not on Pinball Map. Pass one or the other.", + } + ); + +export type UpdateMachineArgs = z.infer; + +export interface MachineFieldChange { + field: string; + from: string | null; + to: string | null; + changed: boolean; +} + +export interface UpdateMachineOutcome extends ToolOutcome { + applied: MachineFieldChange[]; + result: { + initials: string; + name: string; + presence: string; + url: string; + applied: MachineFieldChange[]; + }; +} + +export async function runUpdateMachine( + args: UpdateMachineArgs, + ctx: McpAuthContext +): Promise { + const parsed = updateMachineSchema.safeParse(args); + if (!parsed.success) { + throw new McpToolError( + "invalid", + parsed.error.issues[0]?.message ?? "Invalid arguments." + ); + } + const cleanArgs = parsed.data; + + const machine = await resolveMachine(cleanArgs.machine); + + if ( + !checkPermission("machines.edit", ctx.accessLevel, { + userId: ctx.userId, + machineOwnerId: machine.ownerId, + }) + ) { + throw new McpToolError( + "denied", + "Only the machine owner, technicians, or admins can edit this machine." + ); + } + + const wantsPbm = + cleanArgs.pinballmapMachineId !== undefined || + cleanArgs.pinballmapExcluded !== undefined || + cleanArgs.pinballmapExcludedReason !== undefined || + cleanArgs.intent !== undefined; + + if ( + wantsPbm && + !checkPermission("machines.pinballmap.link", ctx.accessLevel, { + userId: ctx.userId, + machineOwnerId: machine.ownerId, + }) + ) { + throw new McpToolError( + "denied", + "Only the machine owner, technicians, or admins can change this machine's Pinball Map link." + ); + } + + // Pre-resolve owner before applying mutations so invalid owner reference fails cleanly. + const newOwner = + cleanArgs.owner !== undefined + ? await resolveOwner(cleanArgs.owner) + : undefined; + + const applied: MachineFieldChange[] = []; + let currentName = machine.name; + let currentPresenceStatus = machine.presenceStatus; + let currentOwnerId = machine.ownerId; + let currentInvitedOwnerId = machine.invitedOwnerId; + + // 1. updateMachineName (if name supplied) + if (cleanArgs.name !== undefined) { + const { changed } = await updateMachineName({ + machineId: machine.id, + name: cleanArgs.name, + actorUserId: ctx.userId, + current: { + name: currentName, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + presenceStatus: currentPresenceStatus, + }, + }); + applied.push({ + field: "name", + from: currentName, + to: cleanArgs.name, + changed, + }); + currentName = cleanArgs.name; + } + + // 2. updateMachinePresence (if presenceStatus supplied) + if (cleanArgs.presenceStatus !== undefined) { + const { changed } = await updateMachinePresence({ + machineId: machine.id, + presenceStatus: cleanArgs.presenceStatus, + actorUserId: ctx.userId, + current: { + name: currentName, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + presenceStatus: currentPresenceStatus, + }, + }); + applied.push({ + field: "presenceStatus", + from: currentPresenceStatus, + to: cleanArgs.presenceStatus, + changed, + }); + currentPresenceStatus = cleanArgs.presenceStatus; + } + + // 3. updateMachineOwner (if owner supplied) + if (cleanArgs.owner !== undefined && newOwner !== undefined) { + const previousOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + }, + ]); + const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; + + const { deliveryPlan } = await updateMachineOwner({ + machineId: machine.id, + actorUserId: ctx.userId, + current: { + name: currentName, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + presenceStatus: currentPresenceStatus, + }, + newOwner, + }); + + after(() => dispatchNotification(deliveryPlan)); + + const newOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: newOwner.ownerId, + invitedOwnerId: newOwner.invitedOwnerId, + }, + ]); + const toOwnerName = newOwnerNames.get(machine.id) ?? null; + + const changed = + currentOwnerId !== newOwner.ownerId || + currentInvitedOwnerId !== newOwner.invitedOwnerId; + + applied.push({ + field: "owner", + from: fromOwnerName, + to: toOwnerName, + changed, + }); + } + + // 4. updateMachinePbmLink (if pinballmap fields or intent supplied) + if (wantsPbm) { + const updated = await updateMachinePbmLink({ + machineId: machine.id, + actorUserId: ctx.userId, + selection: { + pinballmapMachineId: cleanArgs.pinballmapMachineId, + pinballmapExcluded: cleanArgs.pinballmapExcluded, + pinballmapExcludedReason: cleanArgs.pinballmapExcludedReason, + intent: cleanArgs.intent, + }, + }); + + if (!updated.ok) { + throw new McpToolError(updated.reason, updated.message); + } + + if (cleanArgs.pinballmapMachineId !== undefined) { + applied.push({ + field: "pinballmapMachineId", + from: + updated.previous.pinballmapMachineId !== null + ? String(updated.previous.pinballmapMachineId) + : null, + to: String(cleanArgs.pinballmapMachineId), + changed: + updated.previous.pinballmapMachineId !== + cleanArgs.pinballmapMachineId, + }); + } + + if (cleanArgs.pinballmapExcluded !== undefined) { + const changed = + updated.previous.pinballmapExcluded !== true || + (cleanArgs.pinballmapExcludedReason !== undefined && + updated.previous.pinballmapExcludedReason !== + cleanArgs.pinballmapExcludedReason); + applied.push({ + field: "pinballmapExcluded", + from: updated.previous.pinballmapExcluded ? "true" : "false", + to: "true", + changed, + }); + } + + if (cleanArgs.intent !== undefined) { + applied.push({ + field: "intent", + from: updated.previous.pinballmapIntent, + to: cleanArgs.intent, + changed: updated.previous.pinballmapIntent !== cleanArgs.intent, + }); + } + } + + // 5. updateMachineIscoredLink (if iscoredGameId supplied) + if (cleanArgs.iscoredGameId !== undefined) { + const { changed, iscoredGameId, previousIscoredGameId } = + await updateMachineIscoredLink({ + machineId: machine.id, + iscoredGameId: cleanArgs.iscoredGameId, + }); + applied.push({ + field: "iscoredGameId", + from: previousIscoredGameId, + to: iscoredGameId, + changed, + }); + } + + return { + applied, + result: { + initials: machine.initials, + name: currentName, + presence: currentPresenceStatus, + url: machineUrl(machine.initials), + applied, + }, + machineId: machine.id, + }; +} + +export function registerUpdateMachine(server: McpServer): void { + server.registerTool( + "update_machine", + { + title: "Update a machine", + description: + "Update one or more fields on a machine: name, availability (presenceStatus), owner, Pinball Map link/intent, or iScored link. Supply machine (initials or UUID) and at least one field to change. Returns applied changes.", + inputSchema: updateMachineSchema, + annotations: WRITE_TOOL_ANNOTATIONS, + }, + (args, extra) => + runTool("update_machine", extra, (ctx) => runUpdateMachine(args, ctx), { + mutates: true, + }) + ); +} diff --git a/src/test/integration/mcp-tools.test.ts b/src/test/integration/mcp-tools.test.ts index 4b9f20923..878afa494 100644 --- a/src/test/integration/mcp-tools.test.ts +++ b/src/test/integration/mcp-tools.test.ts @@ -88,6 +88,7 @@ import { import { updateMachineSchema } from "~/app/(app)/m/schemas"; import { updateMachinePbmLink } from "~/services/machines"; import { runUpdateIssue } from "~/lib/mcp/tools/update-issue"; +import { runUpdateMachine } from "~/lib/mcp/tools/update-machine"; import { McpToolError, resolveAssignee, @@ -2812,12 +2813,8 @@ describe("MCP tool handlers (PP-u4ab.2)", () => { "list_issues", "list_machines", "search_pinballmap_catalog", - "set_machine_availability", - "set_machine_iscored", - "set_machine_name", - "set_machine_owner", - "set_machine_pinballmap", "update_issue", + "update_machine", ]); }); @@ -3592,4 +3589,458 @@ describe("MCP tool handlers (PP-u4ab.2)", () => { } }); }); + + describe("update_machine", () => { + it("updates machine name individually and detects no-op", async () => { + const admin = await makeUser("admin"); + const machine = await seedMachine({ name: "Old Name" }); + + const outcome = await runUpdateMachine( + { machine: machine.initials, name: "New Name" }, + ctx("admin", admin) + ); + + expect(outcome.applied).toEqual([ + { + field: "name", + from: "Old Name", + to: "New Name", + changed: true, + }, + ]); + expect(outcome.result.name).toBe("New Name"); + + const db = await getTestDb(); + const updated = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(updated?.name).toBe("New Name"); + + // No-op when unchanged + const noop = await runUpdateMachine( + { machine: machine.initials, name: "New Name" }, + ctx("admin", admin) + ); + expect(noop.applied).toEqual([ + { + field: "name", + from: "New Name", + to: "New Name", + changed: false, + }, + ]); + }); + + it("updates machine presenceStatus individually and detects no-op", async () => { + const admin = await makeUser("admin"); + const machine = await seedMachine({ presenceStatus: "on_the_floor" }); + + const outcome = await runUpdateMachine( + { machine: machine.initials, presenceStatus: "off_the_floor" }, + ctx("admin", admin) + ); + + expect(outcome.applied).toEqual([ + { + field: "presenceStatus", + from: "on_the_floor", + to: "off_the_floor", + changed: true, + }, + ]); + expect(outcome.result.presence).toBe("off_the_floor"); + + const db = await getTestDb(); + const updated = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(updated?.presenceStatus).toBe("off_the_floor"); + + // No-op + const noop = await runUpdateMachine( + { machine: machine.initials, presenceStatus: "off_the_floor" }, + ctx("admin", admin) + ); + expect(noop.applied).toEqual([ + { + field: "presenceStatus", + from: "off_the_floor", + to: "off_the_floor", + changed: false, + }, + ]); + }); + + it("updates machine owner by name, UUID, and clears owner", async () => { + const admin = await makeUser("admin"); + const member1 = await makeUser("member", "Ada", "Lovelace"); + const member2 = await makeUser("member", "Grace", "Hopper"); + const machine = await seedMachine(); + + // Set owner by member full name + const outcome1 = await runUpdateMachine( + { machine: machine.initials, owner: "Ada Lovelace" }, + ctx("admin", admin) + ); + expect(outcome1.applied).toEqual([ + { + field: "owner", + from: null, + to: "Ada Lovelace", + changed: true, + }, + ]); + + const db = await getTestDb(); + let row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.ownerId).toBe(member1); + + // Set owner by UUID + const outcome2 = await runUpdateMachine( + { machine: machine.initials, owner: member2 }, + ctx("admin", admin) + ); + expect(outcome2.applied).toEqual([ + { + field: "owner", + from: "Ada Lovelace", + to: "Grace Hopper", + changed: true, + }, + ]); + row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.ownerId).toBe(member2); + + // No-op when setting same owner + const noop = await runUpdateMachine( + { machine: machine.initials, owner: "Grace Hopper" }, + ctx("admin", admin) + ); + expect(noop.applied).toEqual([ + { + field: "owner", + from: "Grace Hopper", + to: "Grace Hopper", + changed: false, + }, + ]); + + // Clear owner with null + const clearNull = await runUpdateMachine( + { machine: machine.initials, owner: null }, + ctx("admin", admin) + ); + expect(clearNull.applied).toEqual([ + { + field: "owner", + from: "Grace Hopper", + to: null, + changed: true, + }, + ]); + row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.ownerId).toBeNull(); + }); + + it("rejects invalid owner names and guest owners", async () => { + const admin = await makeUser("admin"); + const guest = await makeUser("guest", "Guest", "User"); + const machine = await seedMachine(); + + await expect( + runUpdateMachine( + { machine: machine.initials, owner: "Nonexistent Person" }, + ctx("admin", admin) + ) + ).rejects.toMatchObject({ reason: "not_found" }); + + await expect( + runUpdateMachine( + { machine: machine.initials, owner: guest }, + ctx("admin", admin) + ) + ).rejects.toMatchObject({ reason: "invalid" }); + }); + + it("updates pinballmapMachineId individually and detects no-op", async () => { + await seedElviraCatalog(); + const admin = await makeUser("admin"); + const machine = await seedMachine(); + + const outcome = await runUpdateMachine( + { machine: machine.initials, pinballmapMachineId: ELVIRA_PREMIUM_ID }, + ctx("admin", admin) + ); + expect(outcome.applied).toEqual([ + { + field: "pinballmapMachineId", + from: null, + to: String(ELVIRA_PREMIUM_ID), + changed: true, + }, + ]); + + const db = await getTestDb(); + const row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.pinballmapMachineId).toBe(ELVIRA_PREMIUM_ID); + + // Re-link with same id is a no-op + const noop = await runUpdateMachine( + { machine: machine.initials, pinballmapMachineId: ELVIRA_PREMIUM_ID }, + ctx("admin", admin) + ); + expect(noop.applied).toEqual([ + { + field: "pinballmapMachineId", + from: String(ELVIRA_PREMIUM_ID), + to: String(ELVIRA_PREMIUM_ID), + changed: false, + }, + ]); + }); + + it("marks machine as excluded from Pinball Map with reason", async () => { + const admin = await makeUser("admin"); + const machine = await seedMachine(); + + const outcome = await runUpdateMachine( + { + machine: machine.initials, + pinballmapExcluded: true, + pinballmapExcludedReason: "Custom homebrew", + }, + ctx("admin", admin) + ); + expect(outcome.applied).toEqual([ + { + field: "pinballmapExcluded", + from: "false", + to: "true", + changed: true, + }, + ]); + + const db = await getTestDb(); + const row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.pinballmapExcluded).toBe(true); + expect(row?.pinballmapExcludedReason).toBe("Custom homebrew"); + }); + + it("updates lineup intent individually", async () => { + await seedElviraCatalog(); + const admin = await makeUser("admin"); + const machine = await seedMachine({ + pbm: { pinballmapMachineId: ELVIRA_PREMIUM_ID, pinballmapIntent: "on" }, + }); + + const outcome = await runUpdateMachine( + { machine: machine.initials, intent: "off" }, + ctx("admin", admin) + ); + expect(outcome.applied).toEqual([ + { + field: "intent", + from: "on", + to: "off", + changed: true, + }, + ]); + + const db = await getTestDb(); + const row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.pinballmapIntent).toBe("off"); + }); + + it("updates iscoredGameId individually and clears it", async () => { + const admin = await makeUser("admin"); + const machine = await seedMachine(); + + // Set iscoredGameId + const outcome = await runUpdateMachine( + { machine: machine.initials, iscoredGameId: "iscored-456" }, + ctx("admin", admin) + ); + expect(outcome.applied).toEqual([ + { + field: "iscoredGameId", + from: null, + to: "iscored-456", + changed: true, + }, + ]); + + const db = await getTestDb(); + let row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.iscoredGameId).toBe("iscored-456"); + + // No-op when setting identical id + const noop = await runUpdateMachine( + { machine: machine.initials, iscoredGameId: "iscored-456" }, + ctx("admin", admin) + ); + expect(noop.applied).toEqual([ + { + field: "iscoredGameId", + from: "iscored-456", + to: "iscored-456", + changed: false, + }, + ]); + + // Clear with null + const cleared = await runUpdateMachine( + { machine: machine.initials, iscoredGameId: null }, + ctx("admin", admin) + ); + expect(cleared.applied).toEqual([ + { + field: "iscoredGameId", + from: "iscored-456", + to: null, + changed: true, + }, + ]); + row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row?.iscoredGameId).toBeNull(); + }); + + it("updates multiple fields in combination", async () => { + await seedElviraCatalog(); + const admin = await makeUser("admin"); + const member = await makeUser("member", "Hedy", "Lamarr"); + const machine = await seedMachine({ + name: "Old Cabinet", + presenceStatus: "on_the_floor", + }); + + const outcome = await runUpdateMachine( + { + machine: machine.initials, + name: "New Cabinet Name", + presenceStatus: "off_the_floor", + owner: "Hedy Lamarr", + pinballmapMachineId: ELVIRA_PREMIUM_ID, + iscoredGameId: "iscored-combo-789", + }, + ctx("admin", admin) + ); + + expect(outcome.applied).toEqual([ + { + field: "name", + from: "Old Cabinet", + to: "New Cabinet Name", + changed: true, + }, + { + field: "presenceStatus", + from: "on_the_floor", + to: "off_the_floor", + changed: true, + }, + { + field: "owner", + from: null, + to: "Hedy Lamarr", + changed: true, + }, + { + field: "pinballmapMachineId", + from: null, + to: String(ELVIRA_PREMIUM_ID), + changed: true, + }, + { + field: "iscoredGameId", + from: null, + to: "iscored-combo-789", + changed: true, + }, + ]); + + const db = await getTestDb(); + const row = await db.query.machines.findFirst({ + where: eq(machines.id, machine.id), + }); + expect(row).toMatchObject({ + name: "New Cabinet Name", + presenceStatus: "off_the_floor", + ownerId: member, + pinballmapMachineId: ELVIRA_PREMIUM_ID, + iscoredGameId: "iscored-combo-789", + }); + }); + + it("validates schema requirements: at least one field and mutual exclusion", async () => { + const admin = await makeUser("admin"); + const machine = await seedMachine(); + + // No fields supplied + await expect( + runUpdateMachine({ machine: machine.initials }, ctx("admin", admin)) + ).rejects.toMatchObject({ + reason: "invalid", + message: expect.stringMatching(/at least one field/i), + }); + + // Both pinballmapMachineId and pinballmapExcluded: true + await expect( + runUpdateMachine( + { + machine: machine.initials, + pinballmapMachineId: 12345, + pinballmapExcluded: true, + }, + ctx("admin", admin) + ) + ).rejects.toMatchObject({ + reason: "invalid", + message: expect.stringMatching(/both/i), + }); + }); + + it("enforces permission gates: member cannot update another member's machine but can update their own", async () => { + const member1 = await makeUser("member", "User", "One"); + const member2 = await makeUser("member", "User", "Two"); + const machine = await seedMachine({ ownerId: member1 }); + + // Member 2 trying to edit Member 1's machine + await expect( + runUpdateMachine( + { machine: machine.initials, name: "Unauthorized Rename" }, + ctx("member", member2) + ) + ).rejects.toMatchObject({ reason: "denied" }); + + // Member 1 editing their own machine succeeds + const outcome = await runUpdateMachine( + { machine: machine.initials, name: "Authorized Rename" }, + ctx("member", member1) + ); + expect(outcome.applied).toEqual([ + { + field: "name", + from: "Seed Machine", + to: "Authorized Rename", + changed: true, + }, + ]); + }); + }); }); From 71b43a2c7aa22406931fcbefad9e0a9548a197f2 Mon Sep 17 00:00:00 2001 From: Tim Froehlich Date: Thu, 17 Sep 2026 09:10:26 -0500 Subject: [PATCH 2/4] fix(mcp): address CodeRabbit review comments on PR #2143 (PP-u4ab.18) - Mark severity as optional in create_issue tool description - Require pinballmapExcluded when pinballmapExcludedReason is passed - Return partial failure outcome when earlier mutations committed before later failure - Add integration tests for validation and partial failure reporting --- src/lib/mcp/tools/create-issue.ts | 2 +- src/lib/mcp/tools/update-machine.ts | 228 ++++++++++++++++--------- src/test/integration/mcp-tools.test.ts | 47 +++++ 3 files changed, 200 insertions(+), 77 deletions(-) diff --git a/src/lib/mcp/tools/create-issue.ts b/src/lib/mcp/tools/create-issue.ts index cd43ec632..112089bf4 100644 --- a/src/lib/mcp/tools/create-issue.ts +++ b/src/lib/mcp/tools/create-issue.ts @@ -148,7 +148,7 @@ export function registerCreateIssue(server: McpServer): void { { title: "Create issue", description: - "File a new issue on a machine. Requires machine (initials or UUID), title, and severity; accepts optional description, priority, and frequency.", + "File a new issue on a machine. Requires machine (initials or UUID) and title; accepts optional description, severity, priority, and frequency.", inputSchema: createIssueSchema, annotations: WRITE_TOOL_ANNOTATIONS, }, diff --git a/src/lib/mcp/tools/update-machine.ts b/src/lib/mcp/tools/update-machine.ts index 70539cdd1..f735ae993 100644 --- a/src/lib/mcp/tools/update-machine.ts +++ b/src/lib/mcp/tools/update-machine.ts @@ -106,6 +106,14 @@ export const updateMachineSchema = z message: "A machine can't be both linked to a Pinball Map title and marked as not on Pinball Map. Pass one or the other.", } + ) + .refine( + (args) => + args.pinballmapExcludedReason === undefined || + args.pinballmapExcluded === true, + { + message: "pinballmapExcludedReason requires pinballmapExcluded: true.", + } ); export type UpdateMachineArgs = z.infer; @@ -117,6 +125,11 @@ export interface MachineFieldChange { changed: boolean; } +export interface MachineFieldFailure { + field: string; + reason: string; +} + export interface UpdateMachineOutcome extends ToolOutcome { applied: MachineFieldChange[]; result: { @@ -125,6 +138,8 @@ export interface UpdateMachineOutcome extends ToolOutcome { presence: string; url: string; applied: MachineFieldChange[]; + partial?: boolean; + failed?: MachineFieldFailure; }; } @@ -186,6 +201,35 @@ export async function runUpdateMachine( let currentOwnerId = machine.ownerId; let currentInvitedOwnerId = machine.invitedOwnerId; + function handleFailure( + field: string, + reason: string, + errorReason?: + "denied" | "not_found" | "invalid" | "conflict" | "rate_limited" + ): UpdateMachineOutcome { + if (applied.length > 0) { + return { + applied, + result: { + initials: machine.initials, + name: currentName, + presence: currentPresenceStatus, + url: machineUrl(machine.initials), + applied, + partial: true, + failed: { field, reason }, + }, + machineId: machine.id, + auditOutcome: "error", + auditReason: `partial:${field}`, + }; + } + if (errorReason) { + throw new McpToolError(errorReason, reason); + } + throw new Error(reason); + } + // 1. updateMachineName (if name supplied) if (cleanArgs.name !== undefined) { const { changed } = await updateMachineName({ @@ -210,87 +254,111 @@ export async function runUpdateMachine( // 2. updateMachinePresence (if presenceStatus supplied) if (cleanArgs.presenceStatus !== undefined) { - const { changed } = await updateMachinePresence({ - machineId: machine.id, - presenceStatus: cleanArgs.presenceStatus, - actorUserId: ctx.userId, - current: { - name: currentName, - ownerId: currentOwnerId, - invitedOwnerId: currentInvitedOwnerId, - presenceStatus: currentPresenceStatus, - }, - }); - applied.push({ - field: "presenceStatus", - from: currentPresenceStatus, - to: cleanArgs.presenceStatus, - changed, - }); - currentPresenceStatus = cleanArgs.presenceStatus; + try { + const { changed } = await updateMachinePresence({ + machineId: machine.id, + presenceStatus: cleanArgs.presenceStatus, + actorUserId: ctx.userId, + current: { + name: currentName, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + presenceStatus: currentPresenceStatus, + }, + }); + applied.push({ + field: "presenceStatus", + from: currentPresenceStatus, + to: cleanArgs.presenceStatus, + changed, + }); + currentPresenceStatus = cleanArgs.presenceStatus; + } catch (error) { + return handleFailure( + "presenceStatus", + error instanceof Error ? error.message : "Updating presence failed." + ); + } } // 3. updateMachineOwner (if owner supplied) if (cleanArgs.owner !== undefined && newOwner !== undefined) { - const previousOwnerNames = await getOwnerNamesByMachine([ - { - id: machine.id, - ownerId: currentOwnerId, - invitedOwnerId: currentInvitedOwnerId, - }, - ]); - const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; + try { + const previousOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + }, + ]); + const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; - const { deliveryPlan } = await updateMachineOwner({ - machineId: machine.id, - actorUserId: ctx.userId, - current: { - name: currentName, - ownerId: currentOwnerId, - invitedOwnerId: currentInvitedOwnerId, - presenceStatus: currentPresenceStatus, - }, - newOwner, - }); + const { deliveryPlan } = await updateMachineOwner({ + machineId: machine.id, + actorUserId: ctx.userId, + current: { + name: currentName, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + presenceStatus: currentPresenceStatus, + }, + newOwner, + }); - after(() => dispatchNotification(deliveryPlan)); + after(() => dispatchNotification(deliveryPlan)); - const newOwnerNames = await getOwnerNamesByMachine([ - { - id: machine.id, - ownerId: newOwner.ownerId, - invitedOwnerId: newOwner.invitedOwnerId, - }, - ]); - const toOwnerName = newOwnerNames.get(machine.id) ?? null; + const newOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: newOwner.ownerId, + invitedOwnerId: newOwner.invitedOwnerId, + }, + ]); + const toOwnerName = newOwnerNames.get(machine.id) ?? null; - const changed = - currentOwnerId !== newOwner.ownerId || - currentInvitedOwnerId !== newOwner.invitedOwnerId; + const changed = + currentOwnerId !== newOwner.ownerId || + currentInvitedOwnerId !== newOwner.invitedOwnerId; - applied.push({ - field: "owner", - from: fromOwnerName, - to: toOwnerName, - changed, - }); + applied.push({ + field: "owner", + from: fromOwnerName, + to: toOwnerName, + changed, + }); + } catch (error) { + return handleFailure( + "owner", + error instanceof Error ? error.message : "Updating owner failed." + ); + } } // 4. updateMachinePbmLink (if pinballmap fields or intent supplied) if (wantsPbm) { - const updated = await updateMachinePbmLink({ - machineId: machine.id, - actorUserId: ctx.userId, - selection: { - pinballmapMachineId: cleanArgs.pinballmapMachineId, - pinballmapExcluded: cleanArgs.pinballmapExcluded, - pinballmapExcludedReason: cleanArgs.pinballmapExcludedReason, - intent: cleanArgs.intent, - }, - }); + let updated: Awaited>; + try { + updated = await updateMachinePbmLink({ + machineId: machine.id, + actorUserId: ctx.userId, + selection: { + pinballmapMachineId: cleanArgs.pinballmapMachineId, + pinballmapExcluded: cleanArgs.pinballmapExcluded, + pinballmapExcludedReason: cleanArgs.pinballmapExcludedReason, + intent: cleanArgs.intent, + }, + }); + } catch (error) { + return handleFailure( + "pinballmap", + error instanceof Error + ? error.message + : "Updating Pinball Map link failed." + ); + } if (!updated.ok) { - throw new McpToolError(updated.reason, updated.message); + return handleFailure("pinballmap", updated.message, updated.reason); } if (cleanArgs.pinballmapMachineId !== undefined) { @@ -333,17 +401,24 @@ export async function runUpdateMachine( // 5. updateMachineIscoredLink (if iscoredGameId supplied) if (cleanArgs.iscoredGameId !== undefined) { - const { changed, iscoredGameId, previousIscoredGameId } = - await updateMachineIscoredLink({ - machineId: machine.id, - iscoredGameId: cleanArgs.iscoredGameId, + try { + const { changed, iscoredGameId, previousIscoredGameId } = + await updateMachineIscoredLink({ + machineId: machine.id, + iscoredGameId: cleanArgs.iscoredGameId, + }); + applied.push({ + field: "iscoredGameId", + from: previousIscoredGameId, + to: iscoredGameId, + changed, }); - applied.push({ - field: "iscoredGameId", - from: previousIscoredGameId, - to: iscoredGameId, - changed, - }); + } catch (error) { + return handleFailure( + "iscoredGameId", + error instanceof Error ? error.message : "Updating iScored link failed." + ); + } } return { @@ -354,6 +429,7 @@ export async function runUpdateMachine( presence: currentPresenceStatus, url: machineUrl(machine.initials), applied, + partial: false, }, machineId: machine.id, }; diff --git a/src/test/integration/mcp-tools.test.ts b/src/test/integration/mcp-tools.test.ts index 878afa494..7ce4e4516 100644 --- a/src/test/integration/mcp-tools.test.ts +++ b/src/test/integration/mcp-tools.test.ts @@ -4013,6 +4013,53 @@ describe("MCP tool handlers (PP-u4ab.2)", () => { reason: "invalid", message: expect.stringMatching(/both/i), }); + + // pinballmapExcludedReason without pinballmapExcluded: true + await expect( + runUpdateMachine( + { + machine: machine.initials, + pinballmapExcludedReason: "Not in catalog", + }, + ctx("admin", admin) + ) + ).rejects.toMatchObject({ + reason: "invalid", + message: expect.stringMatching( + /pinballmapExcludedReason requires pinballmapExcluded: true/i + ), + }); + }); + + it("reports partial failure when earlier mutations commit before later failure", async () => { + const admin = await makeUser("admin", "Admin", "User"); + const machine = await seedMachine(); + + // pinballmapMachineId pointing to non-existent catalog title will fail in updateMachinePbmLink + const outcome = await runUpdateMachine( + { + machine: machine.initials, + name: "Renamed Before PBM Error", + pinballmapMachineId: 999999999, + }, + ctx("admin", admin) + ); + + expect(outcome.result.partial).toBe(true); + expect(outcome.result.failed).toEqual({ + field: "pinballmap", + reason: expect.any(String), + }); + expect(outcome.applied).toEqual([ + { + field: "name", + from: "Seed Machine", + to: "Renamed Before PBM Error", + changed: true, + }, + ]); + expect(outcome.auditOutcome).toBe("error"); + expect(outcome.auditReason).toBe("partial:pinballmap"); }); it("enforces permission gates: member cannot update another member's machine but can update their own", async () => { From 39f8439aceebe3577462a2a976c1c0603633fb97 Mon Sep 17 00:00:00 2001 From: Tim Froehlich Date: Thu, 17 Sep 2026 09:34:34 -0500 Subject: [PATCH 3/4] fix(mcp): treat post-commit owner notifications and name lookup as best-effort (PP-u4ab.18) - Isolate updateMachineOwner call in main try/catch - Move dispatchNotification and getOwnerNamesByMachine to best-effort block after commit so committed owner mutation is never misreported as failed (CORE-ARCH-012) --- src/lib/mcp/tools/update-machine.ts | 67 +++++++++++++++++------------ 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/src/lib/mcp/tools/update-machine.ts b/src/lib/mcp/tools/update-machine.ts index f735ae993..b3ff4b506 100644 --- a/src/lib/mcp/tools/update-machine.ts +++ b/src/lib/mcp/tools/update-machine.ts @@ -5,6 +5,7 @@ import { after } from "next/server"; import { z } from "zod"; import { dispatchNotification } from "~/lib/notifications"; +import { reportError } from "~/lib/observability/report-error"; import { checkPermission } from "~/lib/permissions/helpers"; import { VALID_MACHINE_PRESENCE_STATUSES } from "~/lib/machines/presence"; import { @@ -283,17 +284,20 @@ export async function runUpdateMachine( // 3. updateMachineOwner (if owner supplied) if (cleanArgs.owner !== undefined && newOwner !== undefined) { - try { - const previousOwnerNames = await getOwnerNamesByMachine([ - { - id: machine.id, - ownerId: currentOwnerId, - invitedOwnerId: currentInvitedOwnerId, - }, - ]); - const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; + const previousOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: currentOwnerId, + invitedOwnerId: currentInvitedOwnerId, + }, + ]); + const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; - const { deliveryPlan } = await updateMachineOwner({ + let deliveryPlan: Awaited< + ReturnType + >["deliveryPlan"]; + try { + const outcome = await updateMachineOwner({ machineId: machine.id, actorUserId: ctx.userId, current: { @@ -304,9 +308,18 @@ export async function runUpdateMachine( }, newOwner, }); + deliveryPlan = outcome.deliveryPlan; + } catch (error) { + return handleFailure( + "owner", + error instanceof Error ? error.message : "Updating owner failed." + ); + } + // Owner row is committed past this point: never report it as failed. + let toOwnerName: string | null = null; + try { after(() => dispatchNotification(deliveryPlan)); - const newOwnerNames = await getOwnerNamesByMachine([ { id: machine.id, @@ -314,24 +327,24 @@ export async function runUpdateMachine( invitedOwnerId: newOwner.invitedOwnerId, }, ]); - const toOwnerName = newOwnerNames.get(machine.id) ?? null; - - const changed = - currentOwnerId !== newOwner.ownerId || - currentInvitedOwnerId !== newOwner.invitedOwnerId; - - applied.push({ - field: "owner", - from: fromOwnerName, - to: toOwnerName, - changed, - }); + toOwnerName = newOwnerNames.get(machine.id) ?? null; } catch (error) { - return handleFailure( - "owner", - error instanceof Error ? error.message : "Updating owner failed." - ); + reportError(error, { + action: "mcp.update_machine.ownerPostCommit", + machineId: machine.id, + }); } + + const changed = + currentOwnerId !== newOwner.ownerId || + currentInvitedOwnerId !== newOwner.invitedOwnerId; + + applied.push({ + field: "owner", + from: fromOwnerName, + to: toOwnerName, + changed, + }); } // 4. updateMachinePbmLink (if pinballmap fields or intent supplied) From 175c38708b7c06a8b44d2e1a56076a3d56c5cbe2 Mon Sep 17 00:00:00 2001 From: Tim Froehlich Date: Thu, 17 Sep 2026 09:56:22 -0500 Subject: [PATCH 4/4] fix(mcp): resolve previous owner name before any mutations commit (PP-u4ab.18) - Move getOwnerNamesByMachine lookup next to resolveOwner before mutations begin - Prevent lookup errors from masking earlier applied mutations (CORE-ARCH-012) --- src/lib/mcp/tools/update-machine.ts | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/lib/mcp/tools/update-machine.ts b/src/lib/mcp/tools/update-machine.ts index b3ff4b506..37683fb12 100644 --- a/src/lib/mcp/tools/update-machine.ts +++ b/src/lib/mcp/tools/update-machine.ts @@ -196,6 +196,18 @@ export async function runUpdateMachine( ? await resolveOwner(cleanArgs.owner) : undefined; + let fromOwnerName: string | null = null; + if (cleanArgs.owner !== undefined && newOwner !== undefined) { + const previousOwnerNames = await getOwnerNamesByMachine([ + { + id: machine.id, + ownerId: machine.ownerId, + invitedOwnerId: machine.invitedOwnerId, + }, + ]); + fromOwnerName = previousOwnerNames.get(machine.id) ?? null; + } + const applied: MachineFieldChange[] = []; let currentName = machine.name; let currentPresenceStatus = machine.presenceStatus; @@ -284,15 +296,6 @@ export async function runUpdateMachine( // 3. updateMachineOwner (if owner supplied) if (cleanArgs.owner !== undefined && newOwner !== undefined) { - const previousOwnerNames = await getOwnerNamesByMachine([ - { - id: machine.id, - ownerId: currentOwnerId, - invitedOwnerId: currentInvitedOwnerId, - }, - ]); - const fromOwnerName = previousOwnerNames.get(machine.id) ?? null; - let deliveryPlan: Awaited< ReturnType >["deliveryPlan"];