From fdbad4b8a9920c7fbb6e27b5289d80e0c3549d2f Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Wed, 12 Aug 2026 09:10:58 -0600 Subject: [PATCH 1/5] Add batch pin writes, update-by-id, bulk delete, and collapsible groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Managing many pins scaled badly: there was no batch write, and `label` was the only match key for an update — so even a pure relabel meant deleting and recreating each pin with every decoration restated. The concrete trigger was 42 tool calls to relabel 21 pins. Pins are addressed by id everywhere except the write path: you can list, delete, and inject a shortcut by id, but not update one. That asymmetry is what forced the delete-and-recreate, since changing a label changed the only handle the writer understood. - `dispatch_pin` accepts an optional `id`. With one, the pin is matched by id and `label` becomes an ordinary editable field, making a rename a single-field patch. Without one, behaviour is unchanged. - New `dispatch_pins` writes a batch in one `mutatePins` transaction — one lock, one write, one `agent.upsert` event instead of N of each. Mode `replace` requires a `group` and rebuilds exactly that group in the order given. There is deliberately no whole-list replace: every destructive batch must name the group it may clear, so a call cannot delete a pin the agent forgot to restate. - `dispatch_delete_pin` also takes `ids` or `group`. - Group headings in the sidebar collapse, with a member count. Groups over 8 pins start collapsed; an explicit choice always wins over that default and persists per agent and group. `type` is now optional rather than defaulting to "string". Defaulting it made a relabel silently demote a shortcut pin to a plain string and drop its icon — found by exercising the tools against a dev instance, not by the tests. An omitted type inherits from the stored pin, and the value is validated against the resolved type rather than the request's. Array-level write logic moves to `pin-write.ts` so the single and batch paths share one definition of matching, merging, and validation. Co-Authored-By: Claude Opus 5 --- apps/server/src/agents/manager.ts | 116 +++++--- apps/server/src/agents/pin-merge.ts | 12 +- apps/server/src/agents/pin-write.ts | 245 +++++++++++++++++ apps/server/src/routes/mcp.ts | 3 + apps/server/src/server.ts | 1 + apps/server/src/server/mcp-handlers.ts | 153 ++++++++--- apps/server/src/shared/mcp/server.ts | 184 +++++++++++-- apps/server/test/mcp-handlers.test.ts | 112 +++++++- apps/server/test/pin-write.test.ts | 254 ++++++++++++++++++ apps/web/src/components/app/media-sidebar.tsx | 1 + .../src/components/app/pins-panel.test.tsx | 78 +++++- apps/web/src/components/app/pins-panel.tsx | 149 +++++++--- apps/web/src/lib/store.ts | 15 ++ e2e/media-sidebar.spec.ts | 46 ++++ 14 files changed, 1233 insertions(+), 136 deletions(-) create mode 100644 apps/server/src/agents/pin-write.ts create mode 100644 apps/server/test/pin-write.test.ts diff --git a/apps/server/src/agents/manager.ts b/apps/server/src/agents/manager.ts index c0edec8a..9c0b6b1b 100644 --- a/apps/server/src/agents/manager.ts +++ b/apps/server/src/agents/manager.ts @@ -38,7 +38,15 @@ import { writeLatestEventIfCurrent, } from "./events.js"; import { runLifecycleHook } from "./lifecycle-hooks.js"; -import { clearBlankPinFields, mergePin } from "./pin-merge.js"; +import { + MAX_PINS, + type PinSpec, + applyPinSpec, + applyPinSpecs, + removePinGroup, + removePinsByIds, + replacePinGroup, +} from "./pin-write.js"; import { validatePinCaption, validatePinShortcutFields, @@ -96,8 +104,6 @@ const CLAUDE_FULL_ACCESS_ARG = "--dangerously-skip-permissions"; * `createAgent({ initialPins })`. Pins also flow into the startup * prompt via `buildStartupPrompt`, so the cap also bounds prompt size. */ -const MAX_PINS = 50; - /** * Validate + de-duplicate the `initialPins` array supplied to * `createAgent`. De-dup is case-insensitive on label with last-write-wins @@ -1135,41 +1141,27 @@ export class AgentManager { } /** - * Update in place when the label already exists, append otherwise. Position + * Update in place when the pin already exists, append otherwise. Position * is deliberately stable: re-pinning to refresh a value must not shuffle the * sidebar out from under the user, and grouped pins would tear apart if an - * update relocated a member. An agent that wants a pin moved deletes it and - * pins it again. + * update relocated a member. + * + * The pin is addressed by `id` when the caller supplies one and by label + * otherwise — see `applyPinSpec`, which both this and the batch path share + * so the two cannot drift apart. */ async upsertPin( id: string, - pin: AgentPin + pin: PinSpec ): Promise<{ agent: AgentRecord; pin: AgentPin; created: boolean }> { - let stored: AgentPin = pin; + // Assigned by the mutation below, which always runs before we read it. + let stored!: AgentPin; let created = true; await this.mutatePins(id, (currentPins) => { - const index = currentPins.findIndex( - (p) => p.label.toLowerCase() === pin.label.toLowerCase() - ); - if (index !== -1) { - const pins = [...currentPins]; - stored = mergePin( - { - ...currentPins[index]!, - id: currentPins[index]!.id ?? randomUUID(), - }, - pin - ); - created = false; - pins[index] = stored; - return pins; - } - if (currentPins.length >= MAX_PINS) { - throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); - } - stored = clearBlankPinFields({ ...pin, id: pin.id ?? randomUUID() }); - created = true; - return [...currentPins, stored]; + const result = applyPinSpec(currentPins, pin); + stored = result.stored; + created = result.created; + return result.pins; }); return { @@ -1179,15 +1171,67 @@ export class AgentManager { }; } - async deletePinById(id: string, pinId: string): Promise { + /** + * Write many pins in one transaction. + * + * The point is atomicity and a single round trip: applying N pins through + * `upsertPin` costs N transactions, N `getAgent` reads and N sidebar + * re-renders, and a failure halfway leaves the set half-applied. + * + * In `replace` mode the named group is rebuilt to contain exactly `specs`, + * in order. There is deliberately no whole-list replace: every destructive + * batch has to name the group it is allowed to clear, so no call can remove + * a pin the agent forgot to restate. + */ + async upsertPins( + id: string, + specs: PinSpec[], + options: { mode?: "merge" | "replace"; group?: string } = {} + ): Promise<{ agent: AgentRecord; pins: AgentPin[] }> { + const mode = options.mode ?? "merge"; + if (mode === "replace" && !options.group?.trim()) { + throw new AgentError( + "Replace mode requires a group to scope the replacement to.", + 400 + ); + } + + let stored: AgentPin[] = []; await this.mutatePins(id, (currentPins) => { - const pins = currentPins.filter((p) => p.id !== pinId); - if (pins.length === currentPins.length) { - throw new AgentError("Pin not found.", 404); - } - return pins; + const result = + mode === "replace" + ? replacePinGroup(currentPins, options.group!, specs) + : applyPinSpecs(currentPins, specs); + stored = result.stored; + return result.pins; }); + return { agent: (await this.getAgent(id)) as AgentRecord, pins: stored }; + } + + async deletePinById(id: string, pinId: string): Promise { + await this.mutatePins(id, (currentPins) => + removePinsByIds(currentPins, [pinId]) + ); + + return (await this.getAgent(id)) as AgentRecord; + } + + /** Delete several pins by id in one transaction; every id must exist. */ + async deletePinsByIds(id: string, pinIds: string[]): Promise { + await this.mutatePins(id, (currentPins) => + removePinsByIds(currentPins, pinIds) + ); + + return (await this.getAgent(id)) as AgentRecord; + } + + /** Clear an entire group in one transaction. */ + async deletePinsByGroup(id: string, group: string): Promise { + await this.mutatePins(id, (currentPins) => + removePinGroup(currentPins, group) + ); + return (await this.getAgent(id)) as AgentRecord; } diff --git a/apps/server/src/agents/pin-merge.ts b/apps/server/src/agents/pin-merge.ts index e6013e5d..cc2bdd64 100644 --- a/apps/server/src/agents/pin-merge.ts +++ b/apps/server/src/agents/pin-merge.ts @@ -1,5 +1,13 @@ import type { AgentPin } from "./types.js"; +/** + * A pin whose type has been resolved to a concrete string but not yet checked + * against the allowed set. Merging happens before validation — the effective + * type can come from the stored pin — so these helpers work at this width and + * the caller narrows to `AgentPin` afterwards. + */ +export type DraftPin = Omit & { type: string }; + /** Decorations that an agent clears by sending an empty string. */ const CLEARABLE_FIELDS = ["caption", "group", "icon"] as const; @@ -15,7 +23,7 @@ const SHORTCUT_ONLY_FIELDS = [ * Optional pin decorations are cleared by passing an empty string — there is * no other way to remove one, since an omitted field means "leave as-is". */ -export function clearBlankPinFields(pin: AgentPin): AgentPin { +export function clearBlankPinFields(pin: T): T { const cleared = { ...pin }; for (const field of CLEARABLE_FIELDS) { if (cleared[field] !== undefined && cleared[field]!.trim() === "") { @@ -33,7 +41,7 @@ export function clearBlankPinFields(pin: AgentPin): AgentPin { * silently lose it. Fields the agent omits keep their stored value; fields it * sends as an empty string are removed. */ -export function mergePin(existing: AgentPin, incoming: AgentPin): AgentPin { +export function mergePin(existing: AgentPin, incoming: DraftPin): DraftPin { const merged = clearBlankPinFields({ ...existing, ...incoming, diff --git a/apps/server/src/agents/pin-write.ts b/apps/server/src/agents/pin-write.ts new file mode 100644 index 00000000..ad4b61a3 --- /dev/null +++ b/apps/server/src/agents/pin-write.ts @@ -0,0 +1,245 @@ +import { randomUUID } from "node:crypto"; + +import { + isPinType, + validatePinShortcutFields, + validatePinValue, +} from "../pins.js"; +import { AgentError } from "./errors.js"; +import { clearBlankPinFields, mergePin, type DraftPin } from "./pin-merge.js"; +import type { AgentPin } from "./types.js"; + +export const MAX_PINS = 50; + +/** + * A pin as an agent submits it. `type` is optional because an update that + * omits it inherits the stored one — defaulting it to "string" would make a + * pure relabel silently demote a shortcut pin and drop its icon. + */ +export type PinSpec = Omit & { type?: string }; + +/** + * Validate the pin that is actually about to be stored, narrowing it. + * + * This runs after the merge rather than on the incoming spec, because the + * effective type may come from the stored pin — validating the request alone + * would check the value against the wrong type. + */ +function validateStoredPin(pin: DraftPin): AgentPin { + if (!isPinType(pin.type)) { + throw new AgentError(`Invalid pin type: ${pin.type}`, 400); + } + const stored: AgentPin = { ...pin, type: pin.type }; + try { + validatePinValue(stored.type, stored.value); + if (stored.type === "shortcut") validatePinShortcutFields(stored); + } catch (error) { + throw new AgentError( + error instanceof Error ? error.message : String(error), + 400 + ); + } + return stored; +} + +/** Resolve a spec against the pin it is updating, inheriting an omitted type. */ +function toStorablePin(spec: PinSpec, inheritedType?: string): DraftPin { + return { ...spec, type: spec.type ?? inheritedType ?? "string" }; +} + +/** Case-insensitive label equality — the historical uniqueness rule. */ +function sameLabel(a: string, b: string): boolean { + return a.toLowerCase() === b.toLowerCase(); +} + +function sameGroup(pin: AgentPin, group: string): boolean { + return (pin.group ?? "").trim().toLowerCase() === group.trim().toLowerCase(); +} + +/** + * Locate the pin a spec addresses. + * + * `id` wins when present: it is the only handle that survives a relabel, so + * matching it first is what makes renaming a patch rather than a delete and + * recreate. An `id` that matches nothing is an error rather than a create — + * a typo'd id should not quietly become a second pin alongside the one the + * agent meant to edit. + */ +function findTarget(pins: AgentPin[], spec: PinSpec): number { + if (spec.id !== undefined) { + const index = pins.findIndex((pin) => pin.id === spec.id); + if (index === -1) { + throw new AgentError(`Pin not found: ${spec.id}`, 404); + } + return index; + } + return pins.findIndex((pin) => sameLabel(pin.label, spec.label)); +} + +/** + * Labels are unique case-insensitively, and the sidebar leans on it. A rename + * is the one write that can break the invariant, so it is checked against + * every pin except the one being edited. + */ +function assertLabelFree( + pins: AgentPin[], + label: string, + exceptIndex: number +): void { + const clash = pins.findIndex( + (pin, index) => index !== exceptIndex && sameLabel(pin.label, label) + ); + if (clash !== -1) { + throw new AgentError(`Another pin already uses the label "${label}".`, 400); + } +} + +export type ApplyPinResult = { + pins: AgentPin[]; + stored: AgentPin; + created: boolean; +}; + +/** + * Apply one pin spec to the array, returning a new array. + * + * Position is deliberately stable on update: re-pinning to refresh a value + * must not shuffle the sidebar out from under the user, and grouped pins + * would tear apart if an update relocated a member. + */ +export function applyPinSpec(pins: AgentPin[], spec: PinSpec): ApplyPinResult { + const index = findTarget(pins, spec); + + if (index !== -1) { + assertLabelFree(pins, spec.label, index); + const existing = pins[index]!; + const stored = validateStoredPin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing.type) + ) + ); + const next = [...pins]; + next[index] = stored; + return { pins: next, stored, created: false }; + } + + if (pins.length >= MAX_PINS) { + throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); + } + const stored = validateStoredPin( + clearBlankPinFields({ ...toStorablePin(spec), id: spec.id ?? randomUUID() }) + ); + return { pins: [...pins, stored], stored, created: true }; +} + +/** Fold a batch of specs through {@link applyPinSpec}, in order. */ +export function applyPinSpecs( + pins: AgentPin[], + specs: PinSpec[] +): { pins: AgentPin[]; stored: AgentPin[] } { + let next = pins; + const stored: AgentPin[] = []; + for (const spec of specs) { + const result = applyPinSpec(next, spec); + next = result.pins; + stored.push(result.stored); + } + return { pins: next, stored }; +} + +/** + * Make `group` contain exactly `specs`, in the order given, without deleting + * anything outside it. + * + * Specs resolve against the whole array (by id, then by label) so an existing + * pin can be pulled into the group and keep its id and decorations; a pin the + * agent names this way moves rather than being duplicated. Members of the + * group that no spec claimed are dropped — that is the whole point of a + * replace — but a pin outside the group is only ever moved by being named, + * never removed. + * + * The rebuilt block sits where the group already started, so replacing a + * group's contents does not make it jump position in the sidebar. + */ +export function replacePinGroup( + pins: AgentPin[], + group: string, + specs: PinSpec[] +): { pins: AgentPin[]; stored: AgentPin[] } { + const claimed = new Set(); + const stored: AgentPin[] = []; + + for (const spec of specs) { + const index = findTarget(pins, spec); + if (index === -1) { + stored.push( + validateStoredPin( + clearBlankPinFields({ + ...toStorablePin(spec), + id: spec.id ?? randomUUID(), + }) + ) + ); + continue; + } + if (claimed.has(index)) { + throw new AgentError( + `Two entries in the batch address the same pin "${pins[index]!.label}".`, + 400 + ); + } + claimed.add(index); + const existing = pins[index]!; + stored.push( + validateStoredPin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing.type) + ) + ) + ); + } + + // Anchor the block where the group already lived, so a replace does not + // relocate it relative to ungrouped pins. + const anchor = pins.findIndex((pin) => sameGroup(pin, group)); + const survivors = pins + .map((pin, index) => ({ pin, index })) + .filter(({ pin, index }) => !claimed.has(index) && !sameGroup(pin, group)); + + const next: AgentPin[] = []; + let inserted = false; + for (const { pin, index } of survivors) { + if (!inserted && anchor !== -1 && index > anchor) { + next.push(...stored); + inserted = true; + } + next.push(pin); + } + if (!inserted) next.push(...stored); + + if (next.length > MAX_PINS) { + throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); + } + return { pins: next, stored }; +} + +/** Remove pins by id. Every id must exist, so a typo surfaces rather than no-ops. */ +export function removePinsByIds(pins: AgentPin[], ids: string[]): AgentPin[] { + const wanted = new Set(ids); + const missing = ids.filter((id) => !pins.some((pin) => pin.id === id)); + if (missing.length > 0) { + throw new AgentError(`Pin not found: ${missing.join(", ")}`, 404); + } + return pins.filter((pin) => !wanted.has(pin.id ?? "")); +} + +/** Remove every pin in a group. An empty group is a 404, matching delete-by-id. */ +export function removePinGroup(pins: AgentPin[], group: string): AgentPin[] { + const next = pins.filter((pin) => !sameGroup(pin, group)); + if (next.length === pins.length) { + throw new AgentError(`No pins in group "${group}".`, 404); + } + return next; +} diff --git a/apps/server/src/routes/mcp.ts b/apps/server/src/routes/mcp.ts index f8eb4818..a2761542 100644 --- a/apps/server/src/routes/mcp.ts +++ b/apps/server/src/routes/mcp.ts @@ -69,6 +69,7 @@ type McpRouteDeps = { mcpAddReviewThreadMessage: unknown; mcpListReviewFeedback: unknown; mcpUpsertPin: unknown; + mcpUpsertPins: unknown; mcpDeletePin: unknown; mcpDeletePinByLabel: unknown; mcpGetParentContext: unknown; @@ -214,6 +215,7 @@ export async function registerMcpRoutes( updateWhiteboard: deps.mcpUpdateWhiteboard, clearWhiteboard: deps.mcpClearWhiteboard, upsertPin: deps.mcpUpsertPin, + upsertPins: deps.mcpUpsertPins, deletePin: deps.mcpDeletePin, deletePinByLabel: deps.mcpDeletePinByLabel, listPins: deps.mcpListPins, @@ -328,6 +330,7 @@ export async function registerMcpRoutes( sendMessage: deps.mcpSendMessage, listAgentsForAgent: deps.mcpListAgentsForAgent, upsertPin: deps.mcpUpsertPin, + upsertPins: deps.mcpUpsertPins, deletePin: deps.mcpDeletePin, deletePinByLabel: deps.mcpDeletePinByLabel, listPins: deps.mcpListPins, diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index 1f094fb2..62b495ea 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -635,6 +635,7 @@ async function registerRoutes() { mcpSendMessage: mcpHandlers.sendMessage, mcpListAgentsForAgent: mcpHandlers.listAgentsForAgent, mcpUpsertPin: mcpHandlers.upsertPin, + mcpUpsertPins: mcpHandlers.upsertPins, mcpDeletePin: mcpHandlers.deletePin, mcpDeletePinByLabel: mcpHandlers.deletePinByLabel, mcpListPins: mcpHandlers.listPins, diff --git a/apps/server/src/server/mcp-handlers.ts b/apps/server/src/server/mcp-handlers.ts index ca1e18ee..bfac59be 100644 --- a/apps/server/src/server/mcp-handlers.ts +++ b/apps/server/src/server/mcp-handlers.ts @@ -5,7 +5,8 @@ import { mkdir, readFile, unlink, writeFile } from "node:fs/promises"; import type { FastifyBaseLogger } from "fastify"; import type { Pool } from "pg"; -import type { AgentManager, AgentPin, AgentRecord } from "../agents/manager.js"; +import type { AgentManager, AgentRecord } from "../agents/manager.js"; +import type { PinSpec } from "../agents/pin-write.js"; import { AgentError } from "../agents/errors.js"; import type { WorktreeCleanupMode } from "../agents/types.js"; import { @@ -181,29 +182,44 @@ async function handleSendNotify( return deps.slackNotifier.sendNotification(agent, input); } -async function handleUpsertPin( - deps: CreateMcpHandlersDeps, - agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } -): Promise<{ pin: PinListing; created: boolean }> { - if (!isPinType(pin.type)) { +type PinInput = { + id?: string; + label: string; + value: string; + type?: string; + caption?: string; + group?: string; + icon?: string; + variant?: string; + confirm?: boolean; + disabled?: boolean; +}; + +/** + * Narrow one pin spec to a storable shape. + * + * Shared by the single and batch write paths so a pin the batch tool accepts + * is exactly a pin `dispatch_pin` would have accepted — a batch must not + * become a way to smuggle in a shape the single-pin validator rejects. + * + * An omitted `type` stays omitted rather than defaulting: the write layer + * inherits the stored pin's type, so relabelling a shortcut cannot silently + * demote it to a plain string and strip its icon. Validation of the value + * happens there too, once the effective type is known. + */ +function toValidatedPin(pin: PinInput): PinSpec { + if (pin.type !== undefined && !isPinType(pin.type)) { throw new Error(`Invalid pin type: ${pin.type}`); } - validatePinValue(pin.type, pin.value); + if (pin.type !== undefined) { + validatePinValue(pin.type, pin.value); + } // Captions and grouping are generic; button styling, confirmation, and the // disabled state only mean anything for shortcut pins — silently dropping - // those elsewhere keeps stored pins honest. + // those elsewhere keeps stored pins honest. With no type given we cannot + // tell yet, so they ride along and `mergePin` strips them if the resolved + // type turns out not to be shortcut. if (pin.caption !== undefined) { validatePinCaption(pin.caption); } @@ -211,24 +227,37 @@ async function handleUpsertPin( if (isShortcut) { validatePinShortcutFields(pin); } + const keepShortcutFields = pin.type === undefined || isShortcut; - const result = await deps.agentManager.upsertPin(agentId, { + return { + ...(pin.id !== undefined ? { id: pin.id } : {}), label: pin.label, value: pin.value, - type: pin.type, + ...(pin.type !== undefined ? { type: pin.type } : {}), ...(pin.caption !== undefined ? { caption: pin.caption } : {}), ...(pin.group !== undefined ? { group: pin.group } : {}), - ...(isShortcut && pin.icon !== undefined ? { icon: pin.icon } : {}), - ...(isShortcut && pin.variant !== undefined + ...(keepShortcutFields && pin.icon !== undefined ? { icon: pin.icon } : {}), + ...(keepShortcutFields && pin.variant !== undefined ? { variant: pin.variant as PinShortcutVariant } : {}), - ...(isShortcut && pin.confirm !== undefined + ...(keepShortcutFields && pin.confirm !== undefined ? { confirm: pin.confirm } : {}), - ...(isShortcut && pin.disabled !== undefined + ...(keepShortcutFields && pin.disabled !== undefined ? { disabled: pin.disabled } : {}), - }); + }; +} + +async function handleUpsertPin( + deps: CreateMcpHandlersDeps, + agentId: string, + pin: PinInput +): Promise<{ pin: PinListing; created: boolean }> { + const result = await deps.agentManager.upsertPin( + agentId, + toValidatedPin(pin) + ); deps.publishUiEvent({ type: "agent.upsert", agent: deps.withStreamFlag(result.agent), @@ -236,12 +265,55 @@ async function handleUpsertPin( return { pin: toPinListing(result.pin), created: result.created }; } +async function handleUpsertPins( + deps: CreateMcpHandlersDeps, + agentId: string, + input: { + pins: PinInput[]; + mode?: "merge" | "replace"; + group?: string; + } +): Promise { + // Validate the whole batch before opening the transaction: a bad entry at + // position 19 should fail the call outright rather than leave the first + // eighteen applied. + const specs = input.pins.map((pin) => + toValidatedPin( + // The top-level group is authoritative in replace mode, so an entry + // cannot disagree with the scope it was submitted under. + input.mode === "replace" ? { ...pin, group: input.group } : pin + ) + ); + + const result = await deps.agentManager.upsertPins(agentId, specs, { + ...(input.mode !== undefined ? { mode: input.mode } : {}), + ...(input.group !== undefined ? { group: input.group } : {}), + }); + deps.publishUiEvent({ + type: "agent.upsert", + agent: deps.withStreamFlag(result.agent), + }); + return (result.agent.pins ?? []).map(toPinListing); +} + async function handleDeletePin( deps: CreateMcpHandlersDeps, agentId: string, - pinId: string + input: { id?: string; ids?: string[]; group?: string } ): Promise { - const agent = await deps.agentManager.deletePinById(agentId, pinId); + const targets = [input.id, input.ids, input.group].filter( + (target) => target !== undefined + ); + if (targets.length !== 1) { + throw new Error("Pass exactly one of id, ids, or group."); + } + + const agent = input.group + ? await deps.agentManager.deletePinsByGroup(agentId, input.group) + : await deps.agentManager.deletePinsByIds( + agentId, + input.ids ?? [input.id!] + ); deps.publishUiEvent({ type: "agent.upsert", agent: deps.withStreamFlag(agent), @@ -928,23 +1000,18 @@ export function createMcpHandlers(deps: CreateMcpHandlersDeps) { sendNotify: (agentId: string, input: NotifyInput) => handleSendNotify(deps, agentId, input), - upsertPin: ( + upsertPin: (agentId: string, pin: PinInput) => + handleUpsertPin(deps, agentId, pin), + + upsertPins: ( agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } - ) => handleUpsertPin(deps, agentId, pin), + input: { pins: PinInput[]; mode?: "merge" | "replace"; group?: string } + ) => handleUpsertPins(deps, agentId, input), - deletePin: (agentId: string, pinId: string) => - handleDeletePin(deps, agentId, pinId), + deletePin: ( + agentId: string, + input: { id?: string; ids?: string[]; group?: string } + ) => handleDeletePin(deps, agentId, input), deletePinByLabel: (agentId: string, label: string) => handleDeletePinByLabel(deps, agentId, label), diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index 724fa426..a4765bcc 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -33,6 +33,21 @@ import { loadRepoTools, type RepoToolParam } from "./repo-tools.js"; import { VALID_PIN_SHORTCUT_ICONS } from "../../pins.js"; import { toToolError } from "./tool-error.js"; +/** One pin spec as an agent supplies it, shared by the single and batch tools. */ +type McpPinInput = { + id?: string; + label: string; + value: string; + /** Omitted on an update means "keep the stored type". */ + type?: string; + caption?: string; + group?: string; + icon?: string; + variant?: string; + confirm?: boolean; + disabled?: boolean; +}; + export type McpAgent = { id: string; cwd: string; @@ -61,6 +76,7 @@ const AGENT_TOOLS = new Set([ "dispatch_rename_session", "dispatch_notify", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -123,6 +139,7 @@ const JOB_TOOLS = new Set([ "dispatch_rename_session", "dispatch_notify", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -176,6 +193,7 @@ const JOB_TOOLS = new Set([ const REVIEW_AGENT_TOOLS = new Set([ "dispatch_event", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -420,19 +438,16 @@ export type McpRequestContext = { >; upsertPin?: ( agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } + pin: McpPinInput ) => Promise<{ pin: PinListing; created: boolean }>; - deletePin?: (agentId: string, pinId: string) => Promise; + upsertPins?: ( + agentId: string, + input: { pins: McpPinInput[]; mode?: "merge" | "replace"; group?: string } + ) => Promise; + deletePin?: ( + agentId: string, + input: { id?: string; ids?: string[]; group?: string } + ) => Promise; deletePinByLabel?: (agentId: string, label: string) => Promise; getWhiteboard?: (agentId: string) => Promise; updateWhiteboard?: ( @@ -559,6 +574,7 @@ async function createDispatchMcpServer( }); if (allowed.has("dispatch_pin")) registerPinTool(server, context); + if (allowed.has("dispatch_pins")) registerBatchPinTool(server, context); if (allowed.has("dispatch_delete_pin")) registerDeletePinTool(server, context); if (allowed.has("dispatch_share")) registerShareTool(server, context); @@ -705,11 +721,18 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { "dispatch_pin", { description: - "Pin a key-value pair to the Dispatch UI for this agent. Pins are displayed in the sidebar so users can quickly find important info. To update a pin, set it again with the same label — fields you omit keep their current value, so you can add a group or change a value without restating the rest; pass an empty string to clear caption, group, or icon. To remove a pin, use dispatch_list_pins followed by dispatch_delete_pin. The delete parameter is retained temporarily only for agents that initialized before this tool upgrade. " + + "Pin a key-value pair to the Dispatch UI for this agent. Pins are displayed in the sidebar so users can quickly find important info. To update a pin, set it again with the same label — fields you omit keep their current value, so you can add a group or change a value without restating the rest; pass an empty string to clear caption, group, or icon. To rename a pin, pass its id from dispatch_list_pins along with the new label. To write several pins at once, use dispatch_pins instead of calling this repeatedly. To remove a pin, use dispatch_list_pins followed by dispatch_delete_pin. The delete parameter is retained temporarily only for agents that initialized before this tool upgrade. " + "Good things to pin: dev server URLs (url), PR links (pr), key files changed (filename), test/build result summaries (string), DB migration names (string), relevant doc or issue links (url), architecture decisions or assumptions (string), short structured summaries (markdown), the specific blocking question when in waiting_user state (string). " + "Use type 'shortcut' to give the user a one-click button that sends a prompt back to you — the label is the button text and the value is the prompt you receive when it is clicked. Good for offering the user a concrete next step (launch this work, re-run that check, pick this approach) instead of asking them to type it. When a shortcut pin is how the user answers a question that is blocking you, also emit a waiting_user event so the agent surfaces as needing attention — the pin is the answer mechanism, not the alert. " + "When a shortcut's action becomes temporarily or permanently unavailable but is still worth showing (e.g. its build already started elsewhere), set disabled: true instead of deleting it — the button greys out and stops accepting clicks. Set the caption to explain why (e.g. 'already building — agt_...'); it renders in place of the normal caption. Send disabled: false to re-enable it later.", inputSchema: { + id: z + .string() + .min(1) + .optional() + .describe( + "Exact pin id from dispatch_list_pins. Pass it to edit that pin specifically — this is the only way to change a pin's label, since without an id the label is what identifies the pin. Omit to match by label." + ), label: z .string() .max(100) @@ -734,9 +757,9 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { "markdown", "shortcut", ]) - .default("string") + .optional() .describe( - "Value type. 'url' renders as a clickable link. 'port' renders as a monospace badge. 'code' renders as a monospace badge. 'pr' renders as a pull request link with a PR icon. 'filename' renders with a file icon in monospace. 'markdown' renders constrained markdown for short summaries. 'shortcut' renders a button that sends `value` to your session when clicked. For list-like types (filename, url, string, port), separate multiple values with commas or newlines." + "Value type, defaulting to 'string' on a new pin. Omit when updating an existing pin and its stored type is kept. 'url' renders as a clickable link. 'port' renders as a monospace badge. 'code' renders as a monospace badge. 'pr' renders as a pull request link with a PR icon. 'filename' renders with a file icon in monospace. 'markdown' renders constrained markdown for short summaries. 'shortcut' renders a button that sends `value` to your session when clicked. For list-like types (filename, url, string, port), separate multiple values with commas or newlines." ), caption: z .string() @@ -799,9 +822,10 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { ); } const { pin, created } = await upsertPin(agentId, { + ...(args.id !== undefined ? { id: args.id } : {}), label: args.label, value: args.value, - type: args.type ?? "string", + ...(args.type !== undefined ? { type: args.type } : {}), ...(args.caption !== undefined ? { caption: args.caption } : {}), ...(args.group !== undefined ? { group: args.group } : {}), ...(args.icon !== undefined ? { icon: args.icon } : {}), @@ -827,6 +851,108 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { ); } +/** + * The per-entry shape for `dispatch_pins`. Field semantics live on + * `dispatch_pin` — restating them here would double what every agent pays in + * context for the pin toolset, so this stays terse and points there. + */ +const batchPinEntrySchema = z.object({ + id: z + .string() + .min(1) + .optional() + .describe("Pin id from dispatch_list_pins. Required to change a label."), + label: z.string().max(100).describe("Display label, or button text."), + value: z.string().max(2000).describe("Value, or the prompt for a shortcut."), + type: z + .enum([ + "string", + "url", + "port", + "code", + "pr", + "filename", + "markdown", + "shortcut", + ]) + .optional() + .describe( + "Defaults to 'string' on a new pin; omit on an update to keep the stored type. See dispatch_pin." + ), + caption: z.string().max(160).optional().describe("One-line caption."), + group: z.string().max(100).optional().describe("Shared heading."), + icon: z + .enum(VALID_PIN_SHORTCUT_ICONS) + .optional() + .describe("Shortcut pins only."), + variant: z + .enum(["default", "primary", "destructive"]) + .optional() + .describe("Shortcut pins only."), + confirm: z.boolean().optional().describe("Shortcut pins only."), + disabled: z.boolean().optional().describe("Shortcut pins only."), +}); + +function registerBatchPinTool( + server: McpServer, + context: McpRequestContext +): void { + if (!context.agent || !context.upsertPins) return; + const agentId = context.agent.id; + const upsertPins = context.upsertPins; + + server.registerTool( + "dispatch_pins", + { + description: + "Write several sidebar pins in one atomic call — use this instead of calling dispatch_pin in a loop. Each entry behaves exactly like dispatch_pin: it updates the pin matching its id (or, with no id, its label) and creates one otherwise, keeping any field you omit. Because an id survives a relabel, relabelling a whole set is one call here rather than a delete and recreate per pin. " + + "Default mode 'merge' leaves pins you did not mention alone. Mode 'replace' requires a group and makes that group contain exactly the entries you pass, in the order you pass them — members you omit are deleted, and nothing outside the group is ever removed. Use replace to reorder a group or rewrite it wholesale; use merge for everything else. Returns the full resulting pin list.", + inputSchema: { + pins: z + .array(batchPinEntrySchema) + .min(1) + .max(50) + .describe("Pins to write, applied in order."), + mode: z + .enum(["merge", "replace"]) + .default("merge") + .describe( + "'merge' updates or creates each entry and touches nothing else. 'replace' rebuilds the named group to be exactly these entries." + ), + group: z + .string() + .max(100) + .optional() + .describe( + "Required by mode 'replace': the only group the call may delete from. Entries are filed under it automatically." + ), + }, + }, + async (args) => { + try { + const pins = await upsertPins(agentId, { + pins: args.pins, + ...(args.mode !== undefined ? { mode: args.mode } : {}), + ...(args.group !== undefined ? { group: args.group } : {}), + }); + // Echo the resulting list, not the request: an agent can then see what + // the batch actually produced — order included — rather than assuming + // its input round-tripped. + return { + content: [ + { + type: "text", + text: `Wrote ${args.pins.length} pin(s). Pins are now: ${JSON.stringify(pins)}`, + }, + ], + }; + } catch (error) { + return toToolError(error); + } + } + ); +} + function registerDeletePinTool( server: McpServer, context: McpRequestContext @@ -838,18 +964,38 @@ function registerDeletePinTool( "dispatch_delete_pin", { description: - "Permanently remove one current sidebar pin by its stable ID. Call dispatch_list_pins first and pass the exact returned id.", + "Permanently remove sidebar pins. Pass exactly one of: 'id' for a single pin, 'ids' for several at once, or 'group' to clear an entire group. Call dispatch_list_pins first and pass exact returned ids.", inputSchema: { id: z .string() .min(1) + .optional() .describe("Exact pin id returned by dispatch_list_pins."), + ids: z + .array(z.string().min(1)) + .min(1) + .optional() + .describe( + "Several exact pin ids, removed together. Every id must exist." + ), + group: z + .string() + .max(100) + .optional() + .describe("Remove every pin filed under this group heading."), }, }, async (args) => { try { - await deletePin(agentId, args.id); - return { content: [{ type: "text", text: `Removed pin ${args.id}.` }] }; + await deletePin(agentId, { + ...(args.id !== undefined ? { id: args.id } : {}), + ...(args.ids !== undefined ? { ids: args.ids } : {}), + ...(args.group !== undefined ? { group: args.group } : {}), + }); + const removed = args.group + ? `group "${args.group}"` + : (args.ids ?? [args.id]).join(", "); + return { content: [{ type: "text", text: `Removed ${removed}.` }] }; } catch (error) { return toToolError(error); } diff --git a/apps/server/test/mcp-handlers.test.ts b/apps/server/test/mcp-handlers.test.ts index fbaafe9f..c8254e89 100644 --- a/apps/server/test/mcp-handlers.test.ts +++ b/apps/server/test/mcp-handlers.test.ts @@ -225,11 +225,31 @@ function createMockDeps() { pin: { id: "pin_url", ...pin }, created: true, })), + upsertPins: vi.fn( + async (id: string, specs: Array>) => ({ + agent: { + id, + name: "test-agent", + pins: specs.map((pin, index) => ({ id: `pin_${index}`, ...pin })), + }, + pins: specs.map((pin, index) => ({ id: `pin_${index}`, ...pin })), + }) + ), deletePinById: vi.fn(async (id: string) => ({ id, name: "test-agent", pins: [], })), + deletePinsByIds: vi.fn(async (id: string) => ({ + id, + name: "test-agent", + pins: [], + })), + deletePinsByGroup: vi.fn(async (id: string) => ({ + id, + name: "test-agent", + pins: [], + })), listMedia: vi.fn(async () => []), }, jobService: { @@ -457,15 +477,101 @@ describe("createMcpHandlers", () => { describe("deletePin", () => { it("deletes pin and publishes event", async () => { - await handlers.deletePin("agt_test1", "pin_123"); - expect(deps.agentManager.deletePinById).toHaveBeenCalledWith( + await handlers.deletePin("agt_test1", { id: "pin_123" }); + expect(deps.agentManager.deletePinsByIds).toHaveBeenCalledWith( "agt_test1", - "pin_123" + ["pin_123"] ); expect(deps.publishUiEvent).toHaveBeenCalledWith( expect.objectContaining({ type: "agent.upsert" }) ); }); + + it("deletes several pins in one call", async () => { + await handlers.deletePin("agt_test1", { ids: ["pin_1", "pin_2"] }); + expect(deps.agentManager.deletePinsByIds).toHaveBeenCalledWith( + "agt_test1", + ["pin_1", "pin_2"] + ); + }); + + it("clears a group", async () => { + await handlers.deletePin("agt_test1", { group: "Ready to build" }); + expect(deps.agentManager.deletePinsByGroup).toHaveBeenCalledWith( + "agt_test1", + "Ready to build" + ); + }); + + it("rejects an ambiguous target", async () => { + // Accepting both would leave it unclear which one actually applied. + await expect( + handlers.deletePin("agt_test1", { id: "pin_1", group: "Group" }) + ).rejects.toThrow(/exactly one/i); + await expect(handlers.deletePin("agt_test1", {})).rejects.toThrow( + /exactly one/i + ); + }); + }); + + describe("upsertPins", () => { + it("writes a batch through one manager call", async () => { + await handlers.upsertPins("agt_test1", { + pins: [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "2", type: "string" }, + ], + }); + expect(deps.agentManager.upsertPins).toHaveBeenCalledWith( + "agt_test1", + [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "2", type: "string" }, + ], + {} + ); + // One event for the whole batch, not one per pin. + expect(deps.publishUiEvent).toHaveBeenCalledTimes(1); + }); + + it("validates every entry before writing any", async () => { + vi.mocked(validatePinValue).mockImplementation((type, value) => { + if (value === "bad") throw new Error("Invalid pin value"); + }); + await expect( + handlers.upsertPins("agt_test1", { + pins: [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "bad", type: "string" }, + ], + }) + ).rejects.toThrow(/Invalid pin value/); + expect(deps.agentManager.upsertPins).not.toHaveBeenCalled(); + }); + + it("stamps the scoping group onto every entry in replace mode", async () => { + // The top-level group is the safety boundary; an entry must not be able + // to disagree with it and land outside the group being replaced. + await handlers.upsertPins("agt_test1", { + mode: "replace", + group: "Ready to build", + pins: [ + { label: "One", value: "1", type: "string", group: "Elsewhere" }, + ], + }); + expect(deps.agentManager.upsertPins).toHaveBeenCalledWith( + "agt_test1", + [ + { + label: "One", + value: "1", + type: "string", + group: "Ready to build", + }, + ], + { mode: "replace", group: "Ready to build" } + ); + }); }); describe("listPins", () => { diff --git a/apps/server/test/pin-write.test.ts b/apps/server/test/pin-write.test.ts new file mode 100644 index 00000000..3f3595ec --- /dev/null +++ b/apps/server/test/pin-write.test.ts @@ -0,0 +1,254 @@ +import { describe, expect, it } from "vitest"; + +import type { AgentPin } from "../src/agents/types.js"; +import { + applyPinSpec, + applyPinSpecs, + removePinGroup, + removePinsByIds, + replacePinGroup, +} from "../src/agents/pin-write.js"; + +function pin(over: Partial & { label: string }): AgentPin { + return { value: "v", type: "string", ...over }; +} + +// applyPinSpec accepts a spec whose type may be omitted; the helper above +// always sets one, so these tests pass raw objects where inheritance matters. + +const existing: AgentPin[] = [ + pin({ id: "pin_a", label: "Alpha", group: "Build" }), + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + pin({ id: "pin_c", label: "Charlie" }), +]; + +describe("applyPinSpec", () => { + it("updates in place when the label already exists", () => { + const result = applyPinSpec(existing, pin({ label: "alpha", value: "2" })); + expect(result.created).toBe(false); + expect(result.pins.map((p) => p.id)).toEqual(["pin_a", "pin_b", "pin_c"]); + expect(result.pins[0]!.value).toBe("2"); + // The group it already had survives an update that did not mention it. + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("appends an unknown label", () => { + const result = applyPinSpec(existing, pin({ label: "Delta" })); + expect(result.created).toBe(true); + expect(result.pins).toHaveLength(4); + expect(result.pins[3]!.id).toBeTruthy(); + }); + + it("renames when addressed by id, keeping position and decorations", () => { + // The whole point of id matching: the label is free to change because it + // is no longer what identifies the pin. + const result = applyPinSpec( + existing, + pin({ id: "pin_a", label: "Renamed" }) + ); + expect(result.created).toBe(false); + expect(result.pins[0]).toMatchObject({ + id: "pin_a", + label: "Renamed", + group: "Build", + }); + expect(result.pins).toHaveLength(3); + }); + + it("rejects an id that matches nothing rather than creating a stray pin", () => { + expect(() => + applyPinSpec(existing, pin({ id: "pin_zz", label: "Ghost" })) + ).toThrow(/not found/i); + }); + + it("rejects a rename onto another pin's label", () => { + // Case-insensitive label uniqueness is load-bearing for the sidebar. + expect(() => + applyPinSpec(existing, pin({ id: "pin_a", label: "bravo" })) + ).toThrow(/already uses the label/i); + }); + + it("allows a no-op relabel of the pin onto its own label", () => { + expect(() => + applyPinSpec(existing, pin({ id: "pin_a", label: "ALPHA" })) + ).not.toThrow(); + }); + + it("inherits the stored type when an update omits it", () => { + // Defaulting an omitted type to "string" would make a pure relabel demote + // a shortcut to a plain string and strip its icon — the exact operation + // this whole id-matching path exists to make cheap. + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + variant: "primary", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it now", + value: "do the thing", + }); + expect(result.stored).toMatchObject({ + type: "shortcut", + icon: "zap", + variant: "primary", + label: "Run it now", + }); + }); + + it("defaults a brand new pin with no type to string", () => { + const result = applyPinSpec([], { label: "Fresh", value: "v" }); + expect(result.stored.type).toBe("string"); + }); + + it("still strips shortcut-only fields when a pin is retyped", () => { + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "string", + }); + expect(result.stored.icon).toBeUndefined(); + }); + + it("validates the value against the inherited type, not the request", () => { + const urls: AgentPin[] = [ + { id: "pin_u", label: "Docs", value: "https://x.dev", type: "url" }, + ]; + expect(() => + applyPinSpec(urls, { id: "pin_u", label: "Docs", value: "not a url" }) + ).toThrow(); + }); +}); + +describe("applyPinSpecs", () => { + it("applies a batch in order and reports each stored pin", () => { + const result = applyPinSpecs(existing, [ + pin({ id: "pin_a", label: "A2" }), + pin({ label: "Delta" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "A2", + "Bravo", + "Charlie", + "Delta", + ]); + expect(result.stored).toHaveLength(2); + }); + + it("leaves pins the batch did not mention alone", () => { + const result = applyPinSpecs(existing, [pin({ label: "Delta" })]); + expect(result.pins.map((p) => p.id)).toContain("pin_c"); + }); +}); + +describe("replacePinGroup", () => { + it("makes the group exactly the given pins, in order", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + pin({ label: "Echo", group: "Build" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "Bravo", + "Echo", + "Charlie", + ]); + }); + + it("drops group members the batch omitted", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + ]); + expect(result.pins.some((p) => p.id === "pin_a")).toBe(false); + }); + + it("never removes a pin outside the group", () => { + // The entire safety argument for replace mode: naming a group bounds the + // blast radius, so an ungrouped pin set hours ago cannot be collateral. + const result = replacePinGroup(existing, "Build", []); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + }); + + it("anchors the rebuilt group where it already sat", () => { + const pins = [ + pin({ id: "pin_top", label: "Top" }), + pin({ id: "pin_g1", label: "G1", group: "Build" }), + pin({ id: "pin_end", label: "End" }), + ]; + const result = replacePinGroup(pins, "Build", [ + pin({ label: "Fresh", group: "Build" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual(["Top", "Fresh", "End"]); + }); + + it("appends a group that does not exist yet", () => { + const result = replacePinGroup(existing, "New", [ + pin({ label: "Foxtrot", group: "New" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "Alpha", + "Bravo", + "Charlie", + "Foxtrot", + ]); + }); + + it("moves a named pin into the group instead of duplicating it", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_c", label: "Charlie", group: "Build" }), + ]); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("rejects two entries addressing the same pin", () => { + expect(() => + replacePinGroup(existing, "Build", [ + pin({ id: "pin_a", label: "One", group: "Build" }), + pin({ id: "pin_a", label: "Two", group: "Build" }), + ]) + ).toThrow(/same pin/i); + }); +}); + +describe("removePinsByIds", () => { + it("removes every listed id", () => { + expect(removePinsByIds(existing, ["pin_a", "pin_c"])).toHaveLength(1); + }); + + it("rejects an unknown id rather than silently no-opping", () => { + expect(() => removePinsByIds(existing, ["pin_a", "nope"])).toThrow( + /not found/i + ); + }); +}); + +describe("removePinGroup", () => { + it("removes every member of the group", () => { + expect(removePinGroup(existing, "Build").map((p) => p.id)).toEqual([ + "pin_c", + ]); + }); + + it("matches the group name case-insensitively", () => { + expect(removePinGroup(existing, "build")).toHaveLength(1); + }); + + it("rejects an empty group", () => { + expect(() => removePinGroup(existing, "Nothing")).toThrow(/no pins/i); + }); +}); diff --git a/apps/web/src/components/app/media-sidebar.tsx b/apps/web/src/components/app/media-sidebar.tsx index 4cbda1b5..1ab20de5 100644 --- a/apps/web/src/components/app/media-sidebar.tsx +++ b/apps/web/src/components/app/media-sidebar.tsx @@ -239,6 +239,7 @@ export function MediaSidebarContent({ selectedAgentName={selectedAgentName} selectedAgentWorkspaceRoot={selectedAgentWorkspaceRoot} agentIsRunning={selectedAgentIsRunning} + {...(selectedAgentId ? { collapseScope: selectedAgentId } : {})} // A shortcut fires a real prompt into a live session, so an // in-flight run blocks its own button until it settles — a // double-click would otherwise send the prompt twice. diff --git a/apps/web/src/components/app/pins-panel.test.tsx b/apps/web/src/components/app/pins-panel.test.tsx index 74bc0a88..42b21a9e 100644 --- a/apps/web/src/components/app/pins-panel.test.tsx +++ b/apps/web/src/components/app/pins-panel.test.tsx @@ -22,6 +22,18 @@ const shortcutPin: AgentPin = { caption: "High priority", }; +/** N shortcut pins sharing one group. Group names are unique per test so the + * persisted collapse atoms cannot leak between them. */ +function groupOf(group: string, count: number): AgentPin[] { + return Array.from({ length: count }, (_, index) => ({ + id: `${group}_${index}`, + label: `${group} pin ${index}`, + value: `prompt ${index}`, + type: "shortcut" as const, + group, + })); +} + function renderPanel( pins: AgentPin[], props: Partial[0]> = {} @@ -171,7 +183,8 @@ describe("shortcut pins", () => { const groups = screen.getAllByTestId("pin-group"); expect(groups).toHaveLength(1); expect(groups[0]!.getAttribute("data-pin-group")).toBe("Ready to build"); - expect(within(groups[0]!).getAllByRole("button")).toHaveLength(2); + // Two shortcuts plus the heading's own collapse toggle. + expect(within(groups[0]!).getAllByRole("button")).toHaveLength(3); }); it("marks the shortcut unavailable when no run handler is wired (e.g. agent history)", () => { @@ -220,6 +233,69 @@ describe("shortcut pins", () => { ); }); + it("shows the member count on the group heading", () => { + renderPanel(groupOf("Counted", 3), { onRunShortcut: vi.fn() }); + + expect(screen.getByTestId("pin-group-count").textContent).toBe("3"); + }); + + it("leaves a small group expanded", () => { + renderPanel(groupOf("Small", 3), { onRunShortcut: vi.fn() }); + + const group = screen.getByTestId("pin-group"); + expect(group.getAttribute("data-pin-group-collapsed")).toBe("false"); + expect(within(group).getAllByTestId("pin-item")).toHaveLength(3); + }); + + it("starts a large group collapsed, hiding its members but not its count", () => { + // A long group otherwise pushes every other group off screen. + renderPanel(groupOf("Large", 12), { onRunShortcut: vi.fn() }); + + const group = screen.getByTestId("pin-group"); + expect(group.getAttribute("data-pin-group-collapsed")).toBe("true"); + expect(within(group).queryAllByTestId("pin-item")).toHaveLength(0); + expect(screen.getByTestId("pin-group-count").textContent).toBe("12"); + }); + + it("toggles a group in both directions", () => { + renderPanel(groupOf("Toggled", 2), { onRunShortcut: vi.fn() }); + + const toggle = screen.getByTestId("pin-group-toggle"); + expect(toggle.getAttribute("aria-expanded")).toBe("true"); + + fireEvent.click(toggle); + expect( + screen.getByTestId("pin-group-toggle").getAttribute("aria-expanded") + ).toBe("false"); + expect(screen.queryAllByTestId("pin-item")).toHaveLength(0); + + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect( + screen.getByTestId("pin-group-toggle").getAttribute("aria-expanded") + ).toBe("true"); + expect(screen.getAllByTestId("pin-item")).toHaveLength(2); + }); + + it("keeps an expanded large group expanded when it gains a pin", () => { + // The size default must not override a choice the user already made, or + // expanding a big group would silently undo itself on the next pin. + const { unmount } = renderPanel(groupOf("Sticky", 12), { + onRunShortcut: vi.fn(), + collapseScope: "agt_sticky", + }); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect(screen.getAllByTestId("pin-item")).toHaveLength(12); + unmount(); + + renderPanel(groupOf("Sticky", 13), { + onRunShortcut: vi.fn(), + collapseScope: "agt_sticky", + }); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + it("blocks a second send while the first is still in flight", () => { const onRunShortcut = vi.fn(); renderPanel([shortcutPin], { onRunShortcut, pendingPinId: "pin_1" }); diff --git a/apps/web/src/components/app/pins-panel.tsx b/apps/web/src/components/app/pins-panel.tsx index b16af97b..04964c11 100644 --- a/apps/web/src/components/app/pins-panel.tsx +++ b/apps/web/src/components/app/pins-panel.tsx @@ -2,6 +2,7 @@ import { AlertTriangle, Ban, Check, + ChevronRight, CornerDownLeft, Copy, FileText, @@ -9,6 +10,7 @@ import { Loader2, Pin, } from "lucide-react"; +import { useAtom } from "jotai"; import { useRef, useState } from "react"; import { FrontTruncatedValue } from "@/components/app/agent-meta"; @@ -27,6 +29,7 @@ import { Markdown } from "@/components/ui/markdown"; import { useCoarsePointer } from "@/hooks/use-coarse-pointer"; import { useCopyText } from "@/hooks/use-copy"; import { splitPinValues } from "@/lib/pins"; +import { pinGroupCollapsedAtomFamily } from "@/lib/store"; import { rewritePinUrl } from "@/lib/rewrite-pin-url"; import { ScrollArea } from "@/components/ui/scroll-area"; import { @@ -590,6 +593,101 @@ function layoutPins(pins: AgentPin[]): PinRow[] { return rows; } +/** + * Groups past this size start collapsed. A sidebar full of one agent's pins + * pushes every other group off screen, and a long group is exactly the case + * where the heading and count are more useful than the members. + */ +const AUTO_COLLAPSE_THRESHOLD = 8; + +type PinGroupProps = { + name: string; + pins: AgentPin[]; + collapseScope: string; + workspaceRoot: string | null; + agentIsRunning?: boolean; + onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; + agentName?: string | null; + pendingPinId?: string | null; + buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; +}; + +function PinGroup({ + name, + pins, + collapseScope, + workspaceRoot, + agentIsRunning, + onRunShortcut, + agentName = null, + pendingPinId = null, + buttonRef, +}: PinGroupProps): JSX.Element { + const headingId = `pin-group-${name.toLowerCase().replace(/\s+/g, "-")}`; + const [choice, setChoice] = useAtom( + pinGroupCollapsedAtomFamily(`${collapseScope}::${name.toLowerCase()}`) + ); + // An explicit choice always beats the size-based default. + const collapsed = choice ?? pins.length > AUTO_COLLAPSE_THRESHOLD; + + return ( +
+ + {collapsed ? null : ( +
+ {pins.map((pin) => ( + + ))} +
+ )} +
+ ); +} + /** * The rendering unit for a set of pins: grouping policy and `PinItem` travel * together, so every consumer gets group headings without re-implementing the @@ -604,6 +702,7 @@ export function PinList({ agentName = null, pendingPinId = null, buttonRef, + collapseScope = "default", }: { pins: AgentPin[]; workspaceRoot: string | null; @@ -612,13 +711,15 @@ export function PinList({ agentName?: string | null; pendingPinId?: string | null; buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; + /** Namespaces persisted collapse state — an agent id, so it survives renames. */ + collapseScope?: string; }): JSX.Element { return ( <> {layoutPins(pins).map((row) => row.kind === "pin" ? ( ) : ( -
-
- {row.name} -
-
- {row.pins.map((pin) => ( - - ))} -
-
+ name={row.name} + pins={row.pins} + collapseScope={collapseScope} + workspaceRoot={workspaceRoot} + agentIsRunning={agentIsRunning} + onRunShortcut={onRunShortcut} + agentName={agentName} + pendingPinId={pendingPinId} + buttonRef={buttonRef} + /> ) )} @@ -673,6 +754,8 @@ type PinsPanelProps = { agentIsRunning?: boolean; onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; pendingPinId?: string | null; + /** Agent id, so persisted group collapse survives a session rename. */ + collapseScope?: string; }; /** @@ -738,6 +821,7 @@ export function PinsPanel({ agentIsRunning, onRunShortcut, pendingPinId = null, + collapseScope, }: PinsPanelProps): JSX.Element { const [pendingShortcutPin, setPendingShortcutPin] = useState( null @@ -801,6 +885,7 @@ export function PinsPanel({ agentName={selectedAgentName} pendingPinId={pendingPinId} buttonRef={registerShortcutButton} + {...(collapseScope !== undefined ? { collapseScope } : {})} /> lastTrigger.current?.focus()} diff --git a/apps/web/src/lib/store.ts b/apps/web/src/lib/store.ts index 85094eba..d2136e72 100644 --- a/apps/web/src/lib/store.ts +++ b/apps/web/src/lib/store.ts @@ -116,6 +116,21 @@ export const whiteboardAgentDrewAtomFamily = atomFamily((_agentId: string) => atom(false) ); +/** + * Whether one pin group is collapsed, keyed by `::`. + * + * `null` means the user has never touched this group, which is distinct from + * having chosen "expanded": large groups start collapsed, and that default has + * to keep applying until the user overrides it — otherwise expanding a group + * and then gaining a pin would silently re-collapse it. + */ +export const pinGroupCollapsedAtomFamily = atomFamily((key: string) => + atomWithLocalStorage( + `dispatch:pinGroupCollapsed:${key}`, + null + ) +); + export type DiffViewType = "unified" | "split"; export const diffViewTypeAtom = atomWithLocalStorage( diff --git a/e2e/media-sidebar.spec.ts b/e2e/media-sidebar.spec.ts index 43bb2c05..69fabb96 100644 --- a/e2e/media-sidebar.spec.ts +++ b/e2e/media-sidebar.spec.ts @@ -627,4 +627,50 @@ test.describe("Media sidebar", () => { .click(); expect((await runResponse).request().method()).toBe("POST"); }); + + test("remembers a collapsed pin group across a reload", async ({ + page, + request, + }) => { + const agent = await createAgentViaAPI(request, { + name: `e2e-agent-pin-groups-${Date.now()}`, + cwd: process.cwd(), + }); + await setAgentPinsViaDB( + agent.id, + Array.from({ length: 3 }, (_, index) => ({ + id: `pin_group_${index}`, + label: `Grouped pin ${index}`, + type: "shortcut" as const, + value: `prompt ${index}`, + group: "Ready to build", + })) + ); + + await loadApp(page); + await openMediaSidebarForAgent(page, agent); + const group = page.getByTestId("pin-group"); + + // The default and the toggle are unit-tested; only a real reload can show + // that the choice actually round-trips through localStorage. + await expect(group).toHaveAttribute("data-pin-group-collapsed", "false"); + await expect(page.getByTestId("pin-group-count")).toHaveText("3"); + await page.getByTestId("pin-group-toggle").click(); + await expect(group).toHaveAttribute("data-pin-group-collapsed", "true"); + + // Collapsed hides the members but must keep the group findable. + await expect(page.getByTestId("pin-item")).toHaveCount(0); + await expect(page.getByTestId("pin-group-count")).toHaveText("3"); + + // What only a reload can prove is that the choice round-trips through + // localStorage rather than living in component state. Reopening the panel + // afterwards is its own can of worms (two sidebar instances, remembered + // tab), so assert the stored value directly — how a collapsed group + // renders is already covered by the pins-panel unit tests. + const key = `dispatch:pinGroupCollapsed:${agent.id}::ready to build`; + await page.reload({ waitUntil: "domcontentloaded" }); + await expect + .poll(() => page.evaluate((k) => localStorage.getItem(k), key)) + .toBe("true"); + }); }); From 48ec7a03ed5a0430be79bc29a94e077e0da59983 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Wed, 12 Aug 2026 09:22:00 -0600 Subject: [PATCH 2/5] Address review: scope leakage, a11y wiring, and two destructive-path gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the frontend-ux and backend-security persona reviews of #946. - `collapseScope` no longer falls back to a literal "default" bucket. It is `string | null`; `null` keeps collapse ephemeral rather than persisting under a shared key, and agent history now passes its `agentId` so its groups are namespaced like everywhere else. - The group heading's `aria-labelledby` now targets the name span rather than the whole button, so the group is announced as "Ready to build" and not as the collapse action plus its count. The member region stays mounted with `hidden` so `aria-controls` always resolves; its members still unmount while collapsed. - `replacePinGroup` resolved every spec against the original array, so two entries creating the same new label both looked unmatched and both were appended — breaking case-insensitive label uniqueness, which the sidebar and label-addressed updates depend on. Uniqueness is now asserted over the finished array. - `dispatch_delete_pin` accepted `group: ""`. Since a missing group compares equal to "", that deleted every ungrouped pin. Blank group names are now rejected at the schema (trim + min length) and again in `removePinGroup`/`replacePinGroup`, so no caller can reach the destructive path without naming a group. Co-Authored-By: Claude Opus 5 --- apps/server/src/agents/pin-write.ts | 36 +++++++- apps/server/src/shared/mcp/server.ts | 4 + apps/server/test/pin-write.test.ts | 41 +++++++++ .../app/agent-history-detail-tabs.tsx | 6 +- apps/web/src/components/app/media-sidebar.tsx | 2 +- .../src/components/app/pins-panel.test.tsx | 56 ++++++++++++ apps/web/src/components/app/pins-panel.tsx | 91 +++++++++++++------ apps/web/src/lib/store.ts | 7 ++ 8 files changed, 210 insertions(+), 33 deletions(-) diff --git a/apps/server/src/agents/pin-write.ts b/apps/server/src/agents/pin-write.ts index ad4b61a3..540db8a8 100644 --- a/apps/server/src/agents/pin-write.ts +++ b/apps/server/src/agents/pin-write.ts @@ -56,6 +56,33 @@ function sameGroup(pin: AgentPin, group: string): boolean { return (pin.group ?? "").trim().toLowerCase() === group.trim().toLowerCase(); } +/** + * A blank group name matches every *ungrouped* pin, so accepting one would + * turn "clear this group" into "delete everything without a heading". Group + * targeting is the safety boundary for the destructive paths — it has to name + * something. + */ +function assertNamedGroup(group: string): void { + if (group.trim() === "") { + throw new AgentError("A group name is required.", 400); + } +} + +/** Enforce case-insensitive label uniqueness across a finished pin array. */ +function assertUniqueLabels(pins: AgentPin[]): void { + const seen = new Set(); + for (const pin of pins) { + const key = pin.label.toLowerCase(); + if (seen.has(key)) { + throw new AgentError( + `Two pins would share the label "${pin.label}".`, + 400 + ); + } + seen.add(key); + } +} + /** * Locate the pin a spec addresses. * @@ -167,6 +194,7 @@ export function replacePinGroup( group: string, specs: PinSpec[] ): { pins: AgentPin[]; stored: AgentPin[] } { + assertNamedGroup(group); const claimed = new Set(); const stored: AgentPin[] = []; @@ -219,6 +247,11 @@ export function replacePinGroup( } if (!inserted) next.push(...stored); + // Each spec resolves against the *original* array, so two creates sharing a + // new label would both look unmatched and slip through — check the finished + // array instead. This also catches a create colliding with a survivor. + assertUniqueLabels(next); + if (next.length > MAX_PINS) { throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); } @@ -235,8 +268,9 @@ export function removePinsByIds(pins: AgentPin[], ids: string[]): AgentPin[] { return pins.filter((pin) => !wanted.has(pin.id ?? "")); } -/** Remove every pin in a group. An empty group is a 404, matching delete-by-id. */ +/** Remove every pin in a group. A group with no members is a 404, matching delete-by-id. */ export function removePinGroup(pins: AgentPin[], group: string): AgentPin[] { + assertNamedGroup(group); const next = pins.filter((pin) => !sameGroup(pin, group)); if (next.length === pins.length) { throw new AgentError(`No pins in group "${group}".`, 404); diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index a4765bcc..a5b2d28d 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -921,6 +921,8 @@ function registerBatchPinTool( ), group: z .string() + .trim() + .min(1) .max(100) .optional() .describe( @@ -980,6 +982,8 @@ function registerDeletePinTool( ), group: z .string() + .trim() + .min(1) .max(100) .optional() .describe("Remove every pin filed under this group heading."), diff --git a/apps/server/test/pin-write.test.ts b/apps/server/test/pin-write.test.ts index 3f3595ec..b001c59a 100644 --- a/apps/server/test/pin-write.test.ts +++ b/apps/server/test/pin-write.test.ts @@ -215,6 +215,35 @@ describe("replacePinGroup", () => { expect(result.pins[0]!.group).toBe("Build"); }); + it("rejects two new entries that would share a label", () => { + // Each spec resolves against the original array, so two creates sharing a + // label both look unmatched — the uniqueness check has to run on the + // finished array, not per spec. + expect(() => + replacePinGroup(existing, "Build", [ + pin({ label: "Duplicate", group: "Build" }), + pin({ label: "duplicate", group: "Build" }), + ]) + ).toThrow(/share the label/i); + }); + + it("treats a label matching an outside pin as a move, not a duplicate", () => { + // Label matching resolves against the whole array, so naming an existing + // pin pulls it into the group rather than creating a second one with the + // same label — which is also why this can't break label uniqueness. + const result = replacePinGroup(existing, "Build", [ + pin({ label: "charlie", group: "Build" }), + ]); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("refuses a blank group rather than treating it as 'ungrouped'", () => { + expect(() => replacePinGroup(existing, " ", [])).toThrow( + /group name is required/i + ); + }); + it("rejects two entries addressing the same pin", () => { expect(() => replacePinGroup(existing, "Build", [ @@ -251,4 +280,16 @@ describe("removePinGroup", () => { it("rejects an empty group", () => { expect(() => removePinGroup(existing, "Nothing")).toThrow(/no pins/i); }); + + it("refuses a blank group name instead of deleting every ungrouped pin", () => { + // sameGroup() treats a missing group as "", so a blank name would match + // every ungrouped pin and quietly turn this into a mass delete. + for (const blank of ["", " "]) { + expect(() => removePinGroup(existing, blank)).toThrow( + /group name is required/i + ); + } + // The ungrouped pin is still there. + expect(existing.some((p) => p.id === "pin_c")).toBe(true); + }); }); diff --git a/apps/web/src/components/app/agent-history-detail-tabs.tsx b/apps/web/src/components/app/agent-history-detail-tabs.tsx index df4a7514..fe34c17d 100644 --- a/apps/web/src/components/app/agent-history-detail-tabs.tsx +++ b/apps/web/src/components/app/agent-history-detail-tabs.tsx @@ -147,7 +147,11 @@ export function DetailTabs({ {tab === "pins" && pins.length > 0 && (
- +
)} {tab === "pins" && pins.length === 0 && ( diff --git a/apps/web/src/components/app/media-sidebar.tsx b/apps/web/src/components/app/media-sidebar.tsx index 1ab20de5..a70de314 100644 --- a/apps/web/src/components/app/media-sidebar.tsx +++ b/apps/web/src/components/app/media-sidebar.tsx @@ -239,7 +239,7 @@ export function MediaSidebarContent({ selectedAgentName={selectedAgentName} selectedAgentWorkspaceRoot={selectedAgentWorkspaceRoot} agentIsRunning={selectedAgentIsRunning} - {...(selectedAgentId ? { collapseScope: selectedAgentId } : {})} + collapseScope={selectedAgentId} // A shortcut fires a real prompt into a live session, so an // in-flight run blocks its own button until it settles — a // double-click would otherwise send the prompt twice. diff --git a/apps/web/src/components/app/pins-panel.test.tsx b/apps/web/src/components/app/pins-panel.test.tsx index 42b21a9e..9b4882a8 100644 --- a/apps/web/src/components/app/pins-panel.test.tsx +++ b/apps/web/src/components/app/pins-panel.test.tsx @@ -296,6 +296,62 @@ describe("shortcut pins", () => { ).toBe("false"); }); + it("ties the toggle to the member region and names the group by its text", () => { + renderPanel(groupOf("Wired", 2), { onRunShortcut: vi.fn() }); + + const toggle = screen.getByTestId("pin-group-toggle"); + const region = screen.getByTestId("pin-group-members"); + // aria-controls must resolve, so the region stays mounted while collapsed. + expect(toggle.getAttribute("aria-controls")).toBe(region.id); + // The group's accessible name is the heading text, not the whole button + // (which also reads out the count and the collapse action). + const group = screen.getByTestId("pin-group"); + const labelId = group.getAttribute("aria-labelledby")!; + expect(document.getElementById(labelId)!.textContent).toBe("Wired"); + }); + + it("keeps the member region mounted and marked hidden when collapsed", () => { + renderPanel(groupOf("HiddenRegion", 12), { onRunShortcut: vi.fn() }); + + const region = screen.getByTestId("pin-group-members"); + expect(region.hasAttribute("hidden")).toBe(true); + expect(within(region).queryAllByTestId("pin-item")).toHaveLength(0); + }); + + it("keeps collapse ephemeral when no scope is given", () => { + // Without a scope there is nothing to namespace by; persisting under a + // shared fallback key would leak one list's state onto every other. + const { unmount } = renderPanel(groupOf("Unscoped", 2)); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("true"); + expect( + Object.keys(localStorage).filter( + (k) => k.includes("__unscoped__") || k.includes("unscoped::") + ) + ).toHaveLength(0); + unmount(); + + renderPanel(groupOf("Unscoped", 2)); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + + it("does not share collapse state between two scopes", () => { + const { unmount } = renderPanel(groupOf("Shared", 2), { + collapseScope: "agt_one", + }); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + unmount(); + + renderPanel(groupOf("Shared", 2), { collapseScope: "agt_two" }); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + it("blocks a second send while the first is still in flight", () => { const onRunShortcut = vi.fn(); renderPanel([shortcutPin], { onRunShortcut, pendingPinId: "pin_1" }); diff --git a/apps/web/src/components/app/pins-panel.tsx b/apps/web/src/components/app/pins-panel.tsx index 04964c11..01ddacb0 100644 --- a/apps/web/src/components/app/pins-panel.tsx +++ b/apps/web/src/components/app/pins-panel.tsx @@ -29,7 +29,10 @@ import { Markdown } from "@/components/ui/markdown"; import { useCoarsePointer } from "@/hooks/use-coarse-pointer"; import { useCopyText } from "@/hooks/use-copy"; import { splitPinValues } from "@/lib/pins"; -import { pinGroupCollapsedAtomFamily } from "@/lib/store"; +import { + UNSCOPED_COLLAPSE_KEY, + pinGroupCollapsedAtomFamily, +} from "@/lib/store"; import { rewritePinUrl } from "@/lib/rewrite-pin-url"; import { ScrollArea } from "@/components/ui/scroll-area"; import { @@ -603,7 +606,7 @@ const AUTO_COLLAPSE_THRESHOLD = 8; type PinGroupProps = { name: string; pins: AgentPin[]; - collapseScope: string; + collapseScope: string | null; workspaceRoot: string | null; agentIsRunning?: boolean; onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; @@ -623,10 +626,23 @@ function PinGroup({ pendingPinId = null, buttonRef, }: PinGroupProps): JSX.Element { - const headingId = `pin-group-${name.toLowerCase().replace(/\s+/g, "-")}`; - const [choice, setChoice] = useAtom( - pinGroupCollapsedAtomFamily(`${collapseScope}::${name.toLowerCase()}`) + const slug = name.toLowerCase().replace(/\s+/g, "-"); + const headingId = `pin-group-${slug}`; + const regionId = `pin-group-members-${slug}`; + + // Persist only when the caller named a scope. Without one there is nothing + // to namespace by, and a shared fallback bucket would leak one list's + // collapse choices onto every other unscoped list. + const persisted = useAtom( + pinGroupCollapsedAtomFamily( + collapseScope === null + ? UNSCOPED_COLLAPSE_KEY + : `${collapseScope}::${name.toLowerCase()}` + ) ); + const ephemeral = useState(null); + const [choice, setChoice] = collapseScope === null ? ephemeral : persisted; + // An explicit choice always beats the size-based default. const collapsed = choice ?? pins.length > AUTO_COLLAPSE_THRESHOLD; @@ -639,13 +655,15 @@ function PinGroup({ // The heading is often the question these shortcuts answer, so it // has to be announced with them rather than as loose text above. role="group" + // Points at the name span, not the button: the group's accessible name + // is the heading text, not "collapse Ready to build, 12". aria-labelledby={headingId} > - {collapsed ? null : ( -
- {pins.map((pin) => ( - - ))} -
- )} + {/* The region stays in the tree so `aria-controls` always resolves and + `hidden` carries the state; its members unmount while collapsed. */} + ); } @@ -702,7 +729,7 @@ export function PinList({ agentName = null, pendingPinId = null, buttonRef, - collapseScope = "default", + collapseScope = null, }: { pins: AgentPin[]; workspaceRoot: string | null; @@ -711,8 +738,12 @@ export function PinList({ agentName?: string | null; pendingPinId?: string | null; buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; - /** Namespaces persisted collapse state — an agent id, so it survives renames. */ - collapseScope?: string; + /** + * Namespaces persisted collapse state — an agent id, so it survives a + * session rename. `null` means don't persist at all: a shared fallback + * bucket would leak one list's collapse choices onto every other list. + */ + collapseScope?: string | null; }): JSX.Element { return ( <> @@ -754,8 +785,8 @@ type PinsPanelProps = { agentIsRunning?: boolean; onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; pendingPinId?: string | null; - /** Agent id, so persisted group collapse survives a session rename. */ - collapseScope?: string; + /** Agent id, so persisted group collapse survives a session rename. Omit to keep collapse ephemeral. */ + collapseScope?: string | null; }; /** @@ -885,7 +916,7 @@ export function PinsPanel({ agentName={selectedAgentName} pendingPinId={pendingPinId} buttonRef={registerShortcutButton} - {...(collapseScope !== undefined ? { collapseScope } : {})} + collapseScope={collapseScope ?? null} /> lastTrigger.current?.focus()} diff --git a/apps/web/src/lib/store.ts b/apps/web/src/lib/store.ts index d2136e72..e66fd024 100644 --- a/apps/web/src/lib/store.ts +++ b/apps/web/src/lib/store.ts @@ -124,6 +124,13 @@ export const whiteboardAgentDrewAtomFamily = atomFamily((_agentId: string) => * to keep applying until the user overrides it — otherwise expanding a group * and then gaining a pin would silently re-collapse it. */ +/** + * Placeholder key for a pin list with no scope. `PinGroup` never writes + * through this atom — it falls back to ephemeral state — but the hook still + * has to be called unconditionally, so it needs a key. + */ +export const UNSCOPED_COLLAPSE_KEY = "__unscoped__"; + export const pinGroupCollapsedAtomFamily = atomFamily((key: string) => atomWithLocalStorage( `dispatch:pinGroupCollapsed:${key}`, From 5f9b33d7e2d563d9b03b14fce24155783228d611 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Wed, 12 Aug 2026 09:39:08 -0600 Subject: [PATCH 3/5] Let an update inherit the stored pin value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `type` was made inheritable so a relabel couldn't silently demote a shortcut pin, but `value` was left required — so a rename still restated the prompt it wasn't changing, and "rename is a one-field patch" wasn't actually true. Same gap, same fix. An omitted `value` now inherits from the pin being updated. It stays mandatory when creating, since a pin with no value has nothing to display; that check moves into `toStorablePin`, which is the one place that knows whether an update or a create is happening. Validation still runs on the merged pin, so an update that changes only the type re-checks the value it inherited. Co-Authored-By: Claude Opus 5 --- apps/server/src/agents/pin-write.ts | 33 +++++++++++++++------ apps/server/src/server/mcp-handlers.ts | 8 +++-- apps/server/src/shared/mcp/server.ts | 20 +++++++------ apps/server/test/pin-write.test.ts | 41 ++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 21 deletions(-) diff --git a/apps/server/src/agents/pin-write.ts b/apps/server/src/agents/pin-write.ts index 540db8a8..b9d2c599 100644 --- a/apps/server/src/agents/pin-write.ts +++ b/apps/server/src/agents/pin-write.ts @@ -12,11 +12,18 @@ import type { AgentPin } from "./types.js"; export const MAX_PINS = 50; /** - * A pin as an agent submits it. `type` is optional because an update that - * omits it inherits the stored one — defaulting it to "string" would make a - * pure relabel silently demote a shortcut pin and drop its icon. + * A pin as an agent submits it. + * + * `type` and `value` are optional because an update that omits either + * inherits the stored one. Defaulting `type` to "string" would make a pure + * relabel silently demote a shortcut pin and drop its icon; requiring `value` + * would make the same relabel restate the prompt it is not changing. Both are + * required in effect when creating, enforced in `toStorablePin`. */ -export type PinSpec = Omit & { type?: string }; +export type PinSpec = Omit & { + type?: string; + value?: string; +}; /** * Validate the pin that is actually about to be stored, narrowing it. @@ -42,9 +49,17 @@ function validateStoredPin(pin: DraftPin): AgentPin { return stored; } -/** Resolve a spec against the pin it is updating, inheriting an omitted type. */ -function toStorablePin(spec: PinSpec, inheritedType?: string): DraftPin { - return { ...spec, type: spec.type ?? inheritedType ?? "string" }; +/** + * Resolve a spec against the pin it is updating, inheriting an omitted type or + * value. With nothing to inherit from we are creating, and a pin with no value + * has nothing to display — so that is the one case where value is mandatory. + */ +function toStorablePin(spec: PinSpec, existing?: AgentPin): DraftPin { + const value = spec.value ?? existing?.value; + if (value === undefined) { + throw new AgentError("value is required when creating a pin.", 400); + } + return { ...spec, value, type: spec.type ?? existing?.type ?? "string" }; } /** Case-insensitive label equality — the historical uniqueness rule. */ @@ -143,7 +158,7 @@ export function applyPinSpec(pins: AgentPin[], spec: PinSpec): ApplyPinResult { const stored = validateStoredPin( mergePin( { ...existing, id: existing.id ?? randomUUID() }, - toStorablePin(spec, existing.type) + toStorablePin(spec, existing) ) ); const next = [...pins]; @@ -223,7 +238,7 @@ export function replacePinGroup( validateStoredPin( mergePin( { ...existing, id: existing.id ?? randomUUID() }, - toStorablePin(spec, existing.type) + toStorablePin(spec, existing) ) ) ); diff --git a/apps/server/src/server/mcp-handlers.ts b/apps/server/src/server/mcp-handlers.ts index bfac59be..4d176234 100644 --- a/apps/server/src/server/mcp-handlers.ts +++ b/apps/server/src/server/mcp-handlers.ts @@ -185,7 +185,7 @@ async function handleSendNotify( type PinInput = { id?: string; label: string; - value: string; + value?: string; type?: string; caption?: string; group?: string; @@ -211,7 +211,9 @@ function toValidatedPin(pin: PinInput): PinSpec { if (pin.type !== undefined && !isPinType(pin.type)) { throw new Error(`Invalid pin type: ${pin.type}`); } - if (pin.type !== undefined) { + // Only a spec carrying both can be checked here; anything relying on an + // inherited type or value is validated in `pin-write` once merged. + if (pin.type !== undefined && pin.value !== undefined) { validatePinValue(pin.type, pin.value); } @@ -232,7 +234,7 @@ function toValidatedPin(pin: PinInput): PinSpec { return { ...(pin.id !== undefined ? { id: pin.id } : {}), label: pin.label, - value: pin.value, + ...(pin.value !== undefined ? { value: pin.value } : {}), ...(pin.type !== undefined ? { type: pin.type } : {}), ...(pin.caption !== undefined ? { caption: pin.caption } : {}), ...(pin.group !== undefined ? { group: pin.group } : {}), diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index a5b2d28d..cb3be854 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -37,7 +37,8 @@ import { toToolError } from "./tool-error.js"; type McpPinInput = { id?: string; label: string; - value: string; + /** Omitted on an update means "keep the stored value". */ + value?: string; /** Omitted on an update means "keep the stored type". */ type?: string; caption?: string; @@ -744,7 +745,7 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { .max(2000) .optional() .describe( - "The value to display. For shortcut pins this is the prompt delivered to your session when the button is clicked." + "The value to display. For shortcut pins this is the prompt delivered to your session when the button is clicked. Required on a new pin; omit on an update to keep the stored value." ), type: z .enum([ @@ -816,15 +817,10 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { content: [{ type: "text", text: `Removed pin \"${args.label}\".` }], }; } - if (args.value === undefined) { - return toToolError( - new Error("value is required when creating or updating a pin.") - ); - } const { pin, created } = await upsertPin(agentId, { ...(args.id !== undefined ? { id: args.id } : {}), label: args.label, - value: args.value, + ...(args.value !== undefined ? { value: args.value } : {}), ...(args.type !== undefined ? { type: args.type } : {}), ...(args.caption !== undefined ? { caption: args.caption } : {}), ...(args.group !== undefined ? { group: args.group } : {}), @@ -863,7 +859,13 @@ const batchPinEntrySchema = z.object({ .optional() .describe("Pin id from dispatch_list_pins. Required to change a label."), label: z.string().max(100).describe("Display label, or button text."), - value: z.string().max(2000).describe("Value, or the prompt for a shortcut."), + value: z + .string() + .max(2000) + .optional() + .describe( + "Value, or the prompt for a shortcut. Required on a new pin; omit on an update to keep the stored value." + ), type: z .enum([ "string", diff --git a/apps/server/test/pin-write.test.ts b/apps/server/test/pin-write.test.ts index b001c59a..8481bf1c 100644 --- a/apps/server/test/pin-write.test.ts +++ b/apps/server/test/pin-write.test.ts @@ -106,6 +106,47 @@ describe("applyPinSpec", () => { expect(result.stored.type).toBe("string"); }); + it("inherits the stored value when an update omits it", () => { + // The point of id matching is a one-field patch; making a relabel restate + // the prompt it is not changing would undercut that. + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it now", + }); + expect(result.stored).toMatchObject({ + label: "Run it now", + value: "do the thing", + type: "shortcut", + icon: "zap", + }); + }); + + it("still requires a value when there is nothing to inherit from", () => { + expect(() => applyPinSpec([], { label: "Fresh" })).toThrow( + /value is required/i + ); + }); + + it("validates an inherited value against a newly given type", () => { + // Changing only the type has to re-check the value it kept, or a url pin + // could be retyped without its value ever being checked as a url. + const strings: AgentPin[] = [ + { id: "pin_x", label: "Thing", value: "not a url", type: "string" }, + ]; + expect(() => + applyPinSpec(strings, { id: "pin_x", label: "Thing", type: "url" }) + ).toThrow(); + }); + it("still strips shortcut-only fields when a pin is retyped", () => { const shortcuts: AgentPin[] = [ { From 6573df2b65f419c18bc52e561e431e0b453e1b85 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Wed, 12 Aug 2026 13:54:09 -0600 Subject: [PATCH 4/5] Address architecture review: invariants to the right layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the architecture-review persona. Two were real defects. **Create never stripped shortcut-only fields.** `mergePin` enforced "a non-shortcut pin carries no shortcut decorations" on the update branch only; both create branches bypassed it. Before `type` became optional the handler's isShortcut gate hid this, but an omitted type now lets icon/variant/confirm/disabled through — so dispatch_pin({label, value, confirm: true}) stored a string pin with confirm on it, and since variant/confirm/disabled aren't clearable by empty string the agent could never remove them. The strip moves out of `mergePin` into `finalizePin`, which every write path runs. **`replacePinGroup` didn't file members under the group.** It rebuilt positions but never wrote `group`, so correctness depended on a compensating map in `handleUpsertPins` — the primitive couldn't honour its own name, and every existing test passed `group` on the specs, so they encoded the caller's behaviour rather than the module's contract. It now applies the group itself and the handler's map is gone. Also: - Hoisted `pinFields` so the two tool schemas share their constraints instead of duplicating them; only descriptions differ per tool. - `upsertPins` no longer returns a `pins` field its caller ignored, and the batch echo is a thin {id, label, group} projection — 50 pins of full values to convey ordering was the wrong trade. - `PinGroup` branches above the hooks into Persisted/Ephemeral wrappers over a presentational view, rather than calling both `useAtom` and `useState`. Drops the placeholder atom that was never written through. - Group DOM ids come from `useId`. Both sidebars are always mounted, so name-derived ids collided and the new `aria-controls` resolved to the other instance's region. - Narrowed `PinSpec["type"]` to `PinType`, so the handler's check is carried by the compiler instead of being re-run downstream. - Moved two JSDoc blocks back onto the symbols they describe. Co-Authored-By: Claude Opus 5 --- apps/server/src/agents/manager.ts | 24 ++-- apps/server/src/agents/pin-merge.ts | 36 +++--- apps/server/src/agents/pin-write.ts | 38 ++++-- apps/server/src/server/mcp-handlers.ts | 29 +++-- apps/server/src/server/pin-listing.ts | 20 ++++ apps/server/src/shared/mcp/server.ts | 131 +++++++++------------ apps/server/test/mcp-handlers.test.ts | 16 +-- apps/server/test/pin-merge.test.ts | 32 +++-- apps/server/test/pin-write.test.ts | 49 ++++++++ apps/web/src/components/app/pins-panel.tsx | 95 ++++++++++----- apps/web/src/lib/store.ts | 17 +-- 11 files changed, 294 insertions(+), 193 deletions(-) diff --git a/apps/server/src/agents/manager.ts b/apps/server/src/agents/manager.ts index 9c0b6b1b..15f4c1b5 100644 --- a/apps/server/src/agents/manager.ts +++ b/apps/server/src/agents/manager.ts @@ -98,12 +98,6 @@ export type { const CODEX_FULL_ACCESS_ARG = "--dangerously-bypass-approvals-and-sandbox"; const CLAUDE_FULL_ACCESS_ARG = "--dangerously-skip-permissions"; -/** - * Maximum number of pins per agent. Enforced by `upsertPin` when adding - * one at a time and by `normalizeInitialPins` when seeding via - * `createAgent({ initialPins })`. Pins also flow into the startup - * prompt via `buildStartupPrompt`, so the cap also bounds prompt size. - */ /** * Validate + de-duplicate the `initialPins` array supplied to * `createAgent`. De-dup is case-insensitive on label with last-write-wins @@ -1187,7 +1181,7 @@ export class AgentManager { id: string, specs: PinSpec[], options: { mode?: "merge" | "replace"; group?: string } = {} - ): Promise<{ agent: AgentRecord; pins: AgentPin[] }> { + ): Promise<{ agent: AgentRecord }> { const mode = options.mode ?? "merge"; if (mode === "replace" && !options.group?.trim()) { throw new AgentError( @@ -1196,17 +1190,13 @@ export class AgentManager { ); } - let stored: AgentPin[] = []; - await this.mutatePins(id, (currentPins) => { - const result = - mode === "replace" - ? replacePinGroup(currentPins, options.group!, specs) - : applyPinSpecs(currentPins, specs); - stored = result.stored; - return result.pins; - }); + await this.mutatePins(id, (currentPins) => + mode === "replace" + ? replacePinGroup(currentPins, options.group!, specs).pins + : applyPinSpecs(currentPins, specs).pins + ); - return { agent: (await this.getAgent(id)) as AgentRecord, pins: stored }; + return { agent: (await this.getAgent(id)) as AgentRecord }; } async deletePinById(id: string, pinId: string): Promise { diff --git a/apps/server/src/agents/pin-merge.ts b/apps/server/src/agents/pin-merge.ts index cc2bdd64..050b6461 100644 --- a/apps/server/src/agents/pin-merge.ts +++ b/apps/server/src/agents/pin-merge.ts @@ -34,27 +34,35 @@ export function clearBlankPinFields(pin: T): T { } /** - * Merge an incoming pin onto the one already stored under the same label. + * Drop decorations the resolved type has no meaning for, and clear the ones + * the agent blanked. + * + * Every write ends here, create and update alike: "the resolved type governs + * which decorations survive" is one rule, and stating it per-branch is how a + * plain pin ends up stored carrying `confirm` that nothing can then remove + * (`variant`/`confirm`/`disabled` aren't clearable by empty string). + */ +export function finalizePin(pin: T): T { + const finalized = clearBlankPinFields(pin); + if (finalized.type !== "shortcut") { + for (const field of SHORTCUT_ONLY_FIELDS) delete finalized[field]; + } + return finalized; +} + +/** + * Merge an incoming pin onto the one already stored. * * Merge rather than replace: an agent re-pinning to change one thing (add a * group, refresh a value) shouldn't have to restate every decoration or * silently lose it. Fields the agent omits keep their stored value; fields it - * sends as an empty string are removed. + * sends as an empty string are removed. Callers run `finalizePin` on the + * result — including to strip shortcut-only fields when a pin is re-typed. */ export function mergePin(existing: AgentPin, incoming: DraftPin): DraftPin { - const merged = clearBlankPinFields({ + return { ...existing, ...incoming, id: existing.id ?? incoming.id, - }); - - // Omitting a field means "keep it", which would otherwise let a shortcut's - // icon/variant/confirm/disabled ride along when the pin is re-typed as - // something else — stale state an agent could see in dispatch_list_pins - // and have no way to clear. - if (merged.type !== "shortcut") { - for (const field of SHORTCUT_ONLY_FIELDS) delete merged[field]; - } - - return merged; + }; } diff --git a/apps/server/src/agents/pin-write.ts b/apps/server/src/agents/pin-write.ts index b9d2c599..a1b35548 100644 --- a/apps/server/src/agents/pin-write.ts +++ b/apps/server/src/agents/pin-write.ts @@ -4,11 +4,18 @@ import { isPinType, validatePinShortcutFields, validatePinValue, + type PinType, } from "../pins.js"; import { AgentError } from "./errors.js"; -import { clearBlankPinFields, mergePin, type DraftPin } from "./pin-merge.js"; +import { finalizePin, mergePin, type DraftPin } from "./pin-merge.js"; import type { AgentPin } from "./types.js"; +/** + * Maximum number of pins per agent. Enforced by every write path here and by + * `normalizeInitialPins` when seeding via `createAgent({ initialPins })`. Pins + * also flow into the startup prompt via `buildStartupPrompt`, so the cap also + * bounds prompt size. + */ export const MAX_PINS = 50; /** @@ -21,7 +28,7 @@ export const MAX_PINS = 50; * required in effect when creating, enforced in `toStorablePin`. */ export type PinSpec = Omit & { - type?: string; + type?: PinType; value?: string; }; @@ -156,9 +163,11 @@ export function applyPinSpec(pins: AgentPin[], spec: PinSpec): ApplyPinResult { assertLabelFree(pins, spec.label, index); const existing = pins[index]!; const stored = validateStoredPin( - mergePin( - { ...existing, id: existing.id ?? randomUUID() }, - toStorablePin(spec, existing) + finalizePin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing) + ) ) ); const next = [...pins]; @@ -170,7 +179,7 @@ export function applyPinSpec(pins: AgentPin[], spec: PinSpec): ApplyPinResult { throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); } const stored = validateStoredPin( - clearBlankPinFields({ ...toStorablePin(spec), id: spec.id ?? randomUUID() }) + finalizePin({ ...toStorablePin(spec), id: spec.id ?? randomUUID() }) ); return { pins: [...pins, stored], stored, created: true }; } @@ -213,12 +222,17 @@ export function replacePinGroup( const claimed = new Set(); const stored: AgentPin[] = []; - for (const spec of specs) { + for (const rawSpec of specs) { + // Filing members under the group is this function's own job — leaving it + // to the caller means the primitive cannot honour its name, and a member + // stored without `group` renders under no heading yet sits in the block, + // invisible to the next replace of that same group. + const spec: PinSpec = { ...rawSpec, group }; const index = findTarget(pins, spec); if (index === -1) { stored.push( validateStoredPin( - clearBlankPinFields({ + finalizePin({ ...toStorablePin(spec), id: spec.id ?? randomUUID(), }) @@ -236,9 +250,11 @@ export function replacePinGroup( const existing = pins[index]!; stored.push( validateStoredPin( - mergePin( - { ...existing, id: existing.id ?? randomUUID() }, - toStorablePin(spec, existing) + finalizePin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing) + ) ) ) ); diff --git a/apps/server/src/server/mcp-handlers.ts b/apps/server/src/server/mcp-handlers.ts index 4d176234..b96323f5 100644 --- a/apps/server/src/server/mcp-handlers.ts +++ b/apps/server/src/server/mcp-handlers.ts @@ -34,7 +34,12 @@ import { validatePinValue, type PinShortcutVariant, } from "../pins.js"; -import { toPinListing, type PinListing } from "./pin-listing.js"; +import { + toPinListing, + toPinSummary, + type PinListing, + type PinSummary, +} from "./pin-listing.js"; import { resolveRepoRoot } from "../shared/git/git-context.js"; import { isMediaFile, isTextFile, resolveMediaDir } from "../shared/media.js"; import type { PublishUiEvent, SendAgentPrompt } from "./mcp-handler-types.js"; @@ -275,27 +280,25 @@ async function handleUpsertPins( mode?: "merge" | "replace"; group?: string; } -): Promise { +): Promise { // Validate the whole batch before opening the transaction: a bad entry at // position 19 should fail the call outright rather than leave the first - // eighteen applied. - const specs = input.pins.map((pin) => - toValidatedPin( - // The top-level group is authoritative in replace mode, so an entry - // cannot disagree with the scope it was submitted under. - input.mode === "replace" ? { ...pin, group: input.group } : pin - ) - ); + // eighteen applied. Replace mode files entries under the scoping group + // itself, so nothing needs stamping here. + const specs = input.pins.map(toValidatedPin); - const result = await deps.agentManager.upsertPins(agentId, specs, { + const { agent } = await deps.agentManager.upsertPins(agentId, specs, { ...(input.mode !== undefined ? { mode: input.mode } : {}), ...(input.group !== undefined ? { group: input.group } : {}), }); deps.publishUiEvent({ type: "agent.upsert", - agent: deps.withStreamFlag(result.agent), + agent: deps.withStreamFlag(agent), }); - return (result.agent.pins ?? []).map(toPinListing); + // A thin projection, not the full listing: the point of the echo is to show + // what the batch produced and in what order, and 50 pins' worth of values + // (2000 chars each) would dwarf that. dispatch_list_pins serves full state. + return (agent.pins ?? []).map(toPinSummary); } async function handleDeletePin( diff --git a/apps/server/src/server/pin-listing.ts b/apps/server/src/server/pin-listing.ts index b4f3d43d..ce1ec326 100644 --- a/apps/server/src/server/pin-listing.ts +++ b/apps/server/src/server/pin-listing.ts @@ -19,6 +19,26 @@ export type PinListing = { disabled?: boolean; }; +/** + * The identity-and-position view of a pin, for echoing back a bulk write. + * Deliberately omits `value` and the decorations: a batch of 50 pins would + * otherwise return tens of KB to say what order things ended up in. + */ +export type PinSummary = { + id: string; + label: string; + group?: string; +}; + +export function toPinSummary(pin: AgentPin): PinSummary { + if (!pin.id) throw new Error("Pin is missing its stable ID."); + return { + id: pin.id, + label: pin.label, + ...(pin.group !== undefined ? { group: pin.group } : {}), + }; +} + export function toPinListing(pin: AgentPin): PinListing { if (!pin.id) throw new Error("Pin is missing its stable ID."); return { diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index cb3be854..2e227060 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -49,6 +49,34 @@ type McpPinInput = { disabled?: boolean; }; +/** + * The constrained field types every pin write shares. `dispatch_pin` and + * `dispatch_pins` build their schemas from these and override only the + * `.describe()` text — duplicating the *constraints* is how a raised cap ends + * up enforced on one tool and silently not the other. + */ +const pinFields = { + id: z.string().min(1), + label: z.string().max(100), + value: z.string().max(2000), + type: z.enum([ + "string", + "url", + "port", + "code", + "pr", + "filename", + "markdown", + "shortcut", + ]), + caption: z.string().max(160), + group: z.string().trim().min(1).max(100), + icon: z.enum(VALID_PIN_SHORTCUT_ICONS), + variant: z.enum(["default", "primary", "destructive"]), + confirm: z.boolean(), + disabled: z.boolean(), +} as const; + export type McpAgent = { id: string; cwd: string; @@ -727,73 +755,48 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { "Use type 'shortcut' to give the user a one-click button that sends a prompt back to you — the label is the button text and the value is the prompt you receive when it is clicked. Good for offering the user a concrete next step (launch this work, re-run that check, pick this approach) instead of asking them to type it. When a shortcut pin is how the user answers a question that is blocking you, also emit a waiting_user event so the agent surfaces as needing attention — the pin is the answer mechanism, not the alert. " + "When a shortcut's action becomes temporarily or permanently unavailable but is still worth showing (e.g. its build already started elsewhere), set disabled: true instead of deleting it — the button greys out and stops accepting clicks. Set the caption to explain why (e.g. 'already building — agt_...'); it renders in place of the normal caption. Send disabled: false to re-enable it later.", inputSchema: { - id: z - .string() - .min(1) + id: pinFields.id .optional() .describe( "Exact pin id from dispatch_list_pins. Pass it to edit that pin specifically — this is the only way to change a pin's label, since without an id the label is what identifies the pin. Omit to match by label." ), - label: z - .string() - .max(100) - .describe( - "Display label for the pin (e.g. 'API Server', 'Vite Dev', 'DB Port'). For shortcut pins this is the button text." - ), - value: z - .string() - .max(2000) + label: pinFields.label.describe( + "Display label for the pin (e.g. 'API Server', 'Vite Dev', 'DB Port'). For shortcut pins this is the button text." + ), + value: pinFields.value .optional() .describe( "The value to display. For shortcut pins this is the prompt delivered to your session when the button is clicked. Required on a new pin; omit on an update to keep the stored value." ), - type: z - .enum([ - "string", - "url", - "port", - "code", - "pr", - "filename", - "markdown", - "shortcut", - ]) + type: pinFields.type .optional() .describe( "Value type, defaulting to 'string' on a new pin. Omit when updating an existing pin and its stored type is kept. 'url' renders as a clickable link. 'port' renders as a monospace badge. 'code' renders as a monospace badge. 'pr' renders as a pull request link with a PR icon. 'filename' renders with a file icon in monospace. 'markdown' renders constrained markdown for short summaries. 'shortcut' renders a button that sends `value` to your session when clicked. For list-like types (filename, url, string, port), separate multiple values with commas or newlines." ), - caption: z - .string() - .max(160) + caption: pinFields.caption .optional() .describe( "A one-line caption rendered under the pin, supporting inline markdown (bold, italic, `code`, strikethrough). Works on any pin type. On shortcut pins it is context for the click, not part of the injected prompt." ), - group: z - .string() - .max(100) + group: pinFields.group .optional() .describe( "Renders this pin under a shared heading with every other pin using the same group name — use it to present a set of related actions, or the question they answer, as one block." ), - icon: z - .enum(VALID_PIN_SHORTCUT_ICONS) + icon: pinFields.icon .optional() .describe("Shortcut pins only: icon shown on the button."), - variant: z - .enum(["default", "primary", "destructive"]) + variant: pinFields.variant .optional() .describe( "Shortcut pins only: button styling. 'primary' for the main suggested action, 'destructive' for dangerous ones, 'default' otherwise." ), - confirm: z - .boolean() + confirm: pinFields.confirm .optional() .describe( "Shortcut pins only: when true, clicking asks the user to confirm and shows them the prompt first. Use for destructive or hard-to-undo actions." ), - disabled: z - .boolean() + disabled: pinFields.disabled .optional() .describe( "Shortcut pins only: when true, the button renders non-interactive instead of being deleted — for an action that's temporarily or permanently unavailable but still worth showing. Pair with a caption explaining why. Send false to re-enable." @@ -853,46 +856,30 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { * context for the pin toolset, so this stays terse and points there. */ const batchPinEntrySchema = z.object({ - id: z - .string() - .min(1) + id: pinFields.id .optional() .describe("Pin id from dispatch_list_pins. Required to change a label."), - label: z.string().max(100).describe("Display label, or button text."), - value: z - .string() - .max(2000) + label: pinFields.label.describe("Display label, or button text."), + value: pinFields.value .optional() .describe( "Value, or the prompt for a shortcut. Required on a new pin; omit on an update to keep the stored value." ), - type: z - .enum([ - "string", - "url", - "port", - "code", - "pr", - "filename", - "markdown", - "shortcut", - ]) + type: pinFields.type .optional() .describe( "Defaults to 'string' on a new pin; omit on an update to keep the stored type. See dispatch_pin." ), - caption: z.string().max(160).optional().describe("One-line caption."), - group: z.string().max(100).optional().describe("Shared heading."), - icon: z - .enum(VALID_PIN_SHORTCUT_ICONS) + caption: pinFields.caption.optional().describe("One-line caption."), + group: pinFields.group .optional() - .describe("Shortcut pins only."), - variant: z - .enum(["default", "primary", "destructive"]) - .optional() - .describe("Shortcut pins only."), - confirm: z.boolean().optional().describe("Shortcut pins only."), - disabled: z.boolean().optional().describe("Shortcut pins only."), + .describe( + "Shared heading. Ignored in replace mode, which files entries under its own group." + ), + icon: pinFields.icon.optional().describe("Shortcut pins only."), + variant: pinFields.variant.optional().describe("Shortcut pins only."), + confirm: pinFields.confirm.optional().describe("Shortcut pins only."), + disabled: pinFields.disabled.optional().describe("Shortcut pins only."), }); function registerBatchPinTool( @@ -921,11 +908,7 @@ function registerBatchPinTool( .describe( "'merge' updates or creates each entry and touches nothing else. 'replace' rebuilds the named group to be exactly these entries." ), - group: z - .string() - .trim() - .min(1) - .max(100) + group: pinFields.group .optional() .describe( "Required by mode 'replace': the only group the call may delete from. Entries are filed under it automatically." @@ -982,11 +965,7 @@ function registerDeletePinTool( .describe( "Several exact pin ids, removed together. Every id must exist." ), - group: z - .string() - .trim() - .min(1) - .max(100) + group: pinFields.group .optional() .describe("Remove every pin filed under this group heading."), }, diff --git a/apps/server/test/mcp-handlers.test.ts b/apps/server/test/mcp-handlers.test.ts index c8254e89..f63eb909 100644 --- a/apps/server/test/mcp-handlers.test.ts +++ b/apps/server/test/mcp-handlers.test.ts @@ -549,9 +549,10 @@ describe("createMcpHandlers", () => { expect(deps.agentManager.upsertPins).not.toHaveBeenCalled(); }); - it("stamps the scoping group onto every entry in replace mode", async () => { - // The top-level group is the safety boundary; an entry must not be able - // to disagree with it and land outside the group being replaced. + it("passes the scoping group through as an option, not per entry", async () => { + // Filing entries under the group is `replacePinGroup`'s own job — the + // handler compensating for it here is what let the primitive drift from + // its own contract. Covered end-to-end in pin-write.test.ts. await handlers.upsertPins("agt_test1", { mode: "replace", group: "Ready to build", @@ -561,14 +562,7 @@ describe("createMcpHandlers", () => { }); expect(deps.agentManager.upsertPins).toHaveBeenCalledWith( "agt_test1", - [ - { - label: "One", - value: "1", - type: "string", - group: "Ready to build", - }, - ], + [{ label: "One", value: "1", type: "string", group: "Elsewhere" }], { mode: "replace", group: "Ready to build" } ); }); diff --git a/apps/server/test/pin-merge.test.ts b/apps/server/test/pin-merge.test.ts index 1b632f06..9ba97492 100644 --- a/apps/server/test/pin-merge.test.ts +++ b/apps/server/test/pin-merge.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; -import { clearBlankPinFields, mergePin } from "../src/agents/pin-merge.js"; +import { + clearBlankPinFields, + finalizePin, + mergePin, +} from "../src/agents/pin-merge.js"; import type { AgentPin } from "../src/agents/types.js"; const existing: AgentPin = { @@ -52,13 +56,17 @@ describe("mergePin", () => { }); it("clears a decoration when the agent sends an empty string", () => { - const merged = mergePin(existing, { - label: "What day is it?", - value: "What day is it today?", - type: "shortcut", - caption: "", - icon: " ", - }); + // Blank-clearing and the shortcut-field strip moved to `finalizePin`, so + // that every write path gets them — not just updates. + const merged = finalizePin( + mergePin(existing, { + label: "What day is it?", + value: "What day is it today?", + type: "shortcut", + caption: "", + icon: " ", + }) + ); expect(merged).not.toHaveProperty("caption"); expect(merged).not.toHaveProperty("icon"); @@ -68,9 +76,11 @@ describe("mergePin", () => { it("drops shortcut-only fields when the pin is re-typed", () => { // Omission means "keep", which would otherwise strand // icon/variant/confirm/disabled on a pin that can no longer use them. - const merged = mergePin( - { ...existing, confirm: true, disabled: true }, - { label: "What day is it?", value: "https://example.com", type: "url" } + const merged = finalizePin( + mergePin( + { ...existing, confirm: true, disabled: true }, + { label: "What day is it?", value: "https://example.com", type: "url" } + ) ); expect(merged).not.toHaveProperty("icon"); diff --git a/apps/server/test/pin-write.test.ts b/apps/server/test/pin-write.test.ts index 8481bf1c..da050bf3 100644 --- a/apps/server/test/pin-write.test.ts +++ b/apps/server/test/pin-write.test.ts @@ -130,6 +130,36 @@ describe("applyPinSpec", () => { }); }); + it("strips shortcut-only fields from a newly created non-shortcut pin", () => { + // The strip used to live only on the update branch, so a create could + // store `confirm`/`variant` on a plain pin — and neither is clearable by + // empty string, so the agent could never remove them afterwards. + const result = applyPinSpec([], { + label: "Plain", + value: "v", + icon: "zap", + variant: "destructive", + confirm: true, + disabled: true, + }); + expect(result.stored.type).toBe("string"); + expect(result.stored.icon).toBeUndefined(); + expect(result.stored.variant).toBeUndefined(); + expect(result.stored.confirm).toBeUndefined(); + expect(result.stored.disabled).toBeUndefined(); + }); + + it("keeps shortcut-only fields on a newly created shortcut pin", () => { + const result = applyPinSpec([], { + label: "Go", + value: "do it", + type: "shortcut", + icon: "zap", + confirm: true, + }); + expect(result.stored).toMatchObject({ icon: "zap", confirm: true }); + }); + it("still requires a value when there is nothing to inherit from", () => { expect(() => applyPinSpec([], { label: "Fresh" })).toThrow( /value is required/i @@ -279,6 +309,25 @@ describe("replacePinGroup", () => { expect(result.pins[0]!.group).toBe("Build"); }); + it("files members under the group without the caller pre-setting it", () => { + // The primitive owns group membership. Leaving it to a compensating map in + // the handler made this pass only because the caller remembered — and a + // member stored without `group` renders under no heading yet sits in the + // block, invisible to the next replace of that same group. + const result = replacePinGroup([], "Build", [ + { label: "a", value: "1" }, + { label: "b", value: "2" }, + ]); + expect(result.pins.map((p) => p.group)).toEqual(["Build", "Build"]); + }); + + it("overrides an entry's own group with the scoping group", () => { + const result = replacePinGroup([], "Build", [ + { label: "a", value: "1", group: "Elsewhere" }, + ]); + expect(result.pins[0]!.group).toBe("Build"); + }); + it("refuses a blank group rather than treating it as 'ungrouped'", () => { expect(() => replacePinGroup(existing, " ", [])).toThrow( /group name is required/i diff --git a/apps/web/src/components/app/pins-panel.tsx b/apps/web/src/components/app/pins-panel.tsx index 01ddacb0..38569bfa 100644 --- a/apps/web/src/components/app/pins-panel.tsx +++ b/apps/web/src/components/app/pins-panel.tsx @@ -11,7 +11,7 @@ import { Pin, } from "lucide-react"; import { useAtom } from "jotai"; -import { useRef, useState } from "react"; +import { useId, useRef, useState } from "react"; import { FrontTruncatedValue } from "@/components/app/agent-meta"; import { type AgentPin } from "@/components/app/types"; @@ -29,10 +29,7 @@ import { Markdown } from "@/components/ui/markdown"; import { useCoarsePointer } from "@/hooks/use-coarse-pointer"; import { useCopyText } from "@/hooks/use-copy"; import { splitPinValues } from "@/lib/pins"; -import { - UNSCOPED_COLLAPSE_KEY, - pinGroupCollapsedAtomFamily, -} from "@/lib/store"; +import { pinGroupCollapsedAtomFamily } from "@/lib/store"; import { rewritePinUrl } from "@/lib/rewrite-pin-url"; import { ScrollArea } from "@/components/ui/scroll-area"; import { @@ -615,36 +612,29 @@ type PinGroupProps = { buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; }; -function PinGroup({ +type PinGroupViewProps = Omit & { + collapsed: boolean; + onToggle: () => void; +}; + +function PinGroupView({ name, pins, - collapseScope, + collapsed, + onToggle, workspaceRoot, agentIsRunning, onRunShortcut, agentName = null, pendingPinId = null, buttonRef, -}: PinGroupProps): JSX.Element { - const slug = name.toLowerCase().replace(/\s+/g, "-"); - const headingId = `pin-group-${slug}`; - const regionId = `pin-group-members-${slug}`; - - // Persist only when the caller named a scope. Without one there is nothing - // to namespace by, and a shared fallback bucket would leak one list's - // collapse choices onto every other unscoped list. - const persisted = useAtom( - pinGroupCollapsedAtomFamily( - collapseScope === null - ? UNSCOPED_COLLAPSE_KEY - : `${collapseScope}::${name.toLowerCase()}` - ) - ); - const ephemeral = useState(null); - const [choice, setChoice] = collapseScope === null ? ephemeral : persisted; - - // An explicit choice always beats the size-based default. - const collapsed = choice ?? pins.length > AUTO_COLLAPSE_THRESHOLD; +}: PinGroupViewProps): JSX.Element { + // Ids must be unique per document, not per list: the desktop and mobile + // sidebars are both always mounted, so a name-derived id would appear twice + // and `aria-controls` would resolve to the other instance's region. + const uid = useId(); + const headingId = `pin-group-${uid}`; + const regionId = `pin-group-members-${uid}`; return (