From 6b0feb3ba563eaa3d247aa3114d1aaff35668377 Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 19 Apr 2026 16:16:11 +0800 Subject: [PATCH] fix(mcp): fail fast on invalid parent_id in element tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-hook review: add_bottom_nav_v0 (and siblings) advertised a parent_id contract that could silently no-op. pen-core's insertNodeInTree returns the original tree unchanged when parentId doesn't match any node (tree-utils.ts:200-234), so batch_design's downstream call produces a success-looking response {results, nodeCount} with an orphaned node that never lands on disk. - tools/element-tool-helpers.ts: new ensureParentExists() helper — loads the target doc via openDocument + resolveDocPath, checks getDocChildren(pageId) with findNodeInTree, throws a descriptive Error listing parent_id and pageId if missing - Apply to all 3 element tools (add_scroll_row_v0 / add_bottom_nav_v0 / add_activity_ring_v0) at the top of each handler, before DSL construction. Null parent_id short-circuits (root insertion is OK) - routes/design-routes.ts: fix misleading "Page id" description on add_bottom_nav_v0.parent_id → now matches siblings ("Target parent node id (must exist...)") - Tests: 4 new (throws on bogus parent_id for each of 3 tools, + positive "inserts under valid parent" for add_scroll_row_v0). 86/86 pen-mcp suite pass Bundle rebuilt. format + tsc green. --- .../__tests__/add-activity-ring-v0.test.ts | 11 ++++ .../src/__tests__/add-bottom-nav-v0.test.ts | 11 ++++ .../src/__tests__/add-scroll-row-v0.test.ts | 55 +++++++++++++++++++ packages/pen-mcp/src/routes/design-routes.ts | 6 +- .../pen-mcp/src/tools/add-activity-ring-v0.ts | 2 + .../pen-mcp/src/tools/add-bottom-nav-v0.ts | 2 + .../pen-mcp/src/tools/add-scroll-row-v0.ts | 2 + .../pen-mcp/src/tools/element-tool-helpers.ts | 32 +++++++++++ 8 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 packages/pen-mcp/src/tools/element-tool-helpers.ts diff --git a/packages/pen-mcp/src/__tests__/add-activity-ring-v0.test.ts b/packages/pen-mcp/src/__tests__/add-activity-ring-v0.test.ts index bc465b8b0..e5288223b 100644 --- a/packages/pen-mcp/src/__tests__/add-activity-ring-v0.test.ts +++ b/packages/pen-mcp/src/__tests__/add-activity-ring-v0.test.ts @@ -127,4 +127,15 @@ describe('add_activity_ring_v0 — structure matches anti-pattern fix', () => { expect((ringId as string).length).toBeGreaterThan(0); expect((textId as string).length).toBeGreaterThan(0); }); + + it('throws when parent_id refers to non-existent node (no silent no-op)', async () => { + const fp = await fresh('ring.op'); + await expect( + handleAddActivityRingV0({ + filePath: fp, + center_text: 'X', + parent_id: 'bogus-parent', + }), + ).rejects.toThrow(/parent_id.*not found/); + }); }); diff --git a/packages/pen-mcp/src/__tests__/add-bottom-nav-v0.test.ts b/packages/pen-mcp/src/__tests__/add-bottom-nav-v0.test.ts index 2c73f8e81..7d9e391c1 100644 --- a/packages/pen-mcp/src/__tests__/add-bottom-nav-v0.test.ts +++ b/packages/pen-mcp/src/__tests__/add-bottom-nav-v0.test.ts @@ -138,4 +138,15 @@ describe('add_bottom_nav_v0 — structure', () => { // 1 bar + 2 tabs + 2 * 2 kids = 7 expect(ids.length).toBe(7); }); + + it('throws when parent_id refers to non-existent node (no silent no-op)', async () => { + const fp = await fresh('nav.op'); + await expect( + handleAddBottomNavV0({ + filePath: fp, + items: [{ title: 'Home', icon: 'home' }], + parent_id: 'bogus-parent', + }), + ).rejects.toThrow(/parent_id.*not found/); + }); }); diff --git a/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts b/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts index 2d182c04b..345a596fb 100644 --- a/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts +++ b/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts @@ -352,6 +352,61 @@ describe('add_scroll_row_v0 — persistence', () => { }); }); +describe('add_scroll_row_v0 — parent_id validation (fail-fast, no silent no-op)', () => { + it('throws when parent_id refers to non-existent node', async () => { + const fp = await fresh('cards.op'); + await expect( + handleAddScrollRowV0({ + filePath: fp, + children_type: 'card', + items: [{ title: 'A' }], + parent_id: 'bogus-id-does-not-exist', + }), + ).rejects.toThrow(/parent_id.*not found/); + }); + + it('inserts under a valid parent_id', async () => { + const fp = await fresh('cards.op'); + // Seed with a container node first + const { writeFile: wf } = await import('node:fs/promises'); + await wf( + fp, + JSON.stringify({ + version: '1.0.0', + children: [ + { + id: 'container-1', + type: 'frame', + name: 'Container', + width: 1200, + height: 0, + layout: 'vertical', + children: [], + }, + ], + }), + 'utf-8', + ); + invalidateCache(fp); + const result = await handleAddScrollRowV0({ + filePath: fp, + children_type: 'card', + items: [{ title: 'A' }, { title: 'B' }], + parent_id: 'container-1', + }); + expect(result.results).toHaveLength(1); + // Read back and verify wrapper was inserted under container-1 + const doc = await readDoc(fp); + const pages = doc['pages'] as Array<{ children?: Record[] }> | undefined; + const topLevel = doc['children'] as Record[] | undefined; + const topChildren = topLevel ?? pages?.[0]?.children ?? []; + const container = topChildren[0]; + expect(container.id).toBe('container-1'); + const wrapper = (container.children as Record[])[0]; + expect(wrapper.role).toBe('scroll-row-wrapper'); + }); +}); + describe('add_scroll_row_v0 — snapshot golden output', () => { it('golden: 3 cards with title+subtitle+icon', async () => { const fp = await fresh('cards.op'); diff --git a/packages/pen-mcp/src/routes/design-routes.ts b/packages/pen-mcp/src/routes/design-routes.ts index 6843c9940..c1fb2c285 100644 --- a/packages/pen-mcp/src/routes/design-routes.ts +++ b/packages/pen-mcp/src/routes/design-routes.ts @@ -160,7 +160,11 @@ export const DESIGN_TOOL_DEFINITIONS = [ }, }, height: { type: 'number', description: 'Bar height in px (default 62)' }, - parent_id: { type: 'string', description: 'Page id; omit for root-level' }, + parent_id: { + type: 'string', + description: + 'Target parent node id (must exist in the document; validated before insertion). Omit for root-level insertion.', + }, pageId: { type: 'string', description: 'Target page ID (defaults to first page)' }, }, required: ['items'], diff --git a/packages/pen-mcp/src/tools/add-activity-ring-v0.ts b/packages/pen-mcp/src/tools/add-activity-ring-v0.ts index 872e35b0d..b3a3ee665 100644 --- a/packages/pen-mcp/src/tools/add-activity-ring-v0.ts +++ b/packages/pen-mcp/src/tools/add-activity-ring-v0.ts @@ -1,5 +1,6 @@ import { handleBatchDesign } from './batch-design'; import { generateId } from '../utils/id'; +import { ensureParentExists } from './element-tool-helpers'; export interface AddActivityRingV0Params { size?: number; @@ -36,6 +37,7 @@ export interface AddActivityRingV0Params { export async function handleAddActivityRingV0( params: AddActivityRingV0Params, ): Promise>> { + await ensureParentExists(params); const size = params.size ?? 80; const thickness = params.thickness ?? 8; const ringColor = params.ring_color ?? '#000000'; diff --git a/packages/pen-mcp/src/tools/add-bottom-nav-v0.ts b/packages/pen-mcp/src/tools/add-bottom-nav-v0.ts index a4d33bbaa..a6d3282cf 100644 --- a/packages/pen-mcp/src/tools/add-bottom-nav-v0.ts +++ b/packages/pen-mcp/src/tools/add-bottom-nav-v0.ts @@ -1,5 +1,6 @@ import { handleBatchDesign } from './batch-design'; import { generateId } from '../utils/id'; +import { ensureParentExists } from './element-tool-helpers'; export interface AddBottomNavV0Item { title: string; @@ -31,6 +32,7 @@ export interface AddBottomNavV0Params { export async function handleAddBottomNavV0( params: AddBottomNavV0Params, ): Promise>> { + await ensureParentExists(params); const height = params.height ?? 62; const nav = buildNav(params, height); assignIdsRecursively(nav); diff --git a/packages/pen-mcp/src/tools/add-scroll-row-v0.ts b/packages/pen-mcp/src/tools/add-scroll-row-v0.ts index 4c0109436..bccecf7fc 100644 --- a/packages/pen-mcp/src/tools/add-scroll-row-v0.ts +++ b/packages/pen-mcp/src/tools/add-scroll-row-v0.ts @@ -1,5 +1,6 @@ import { handleBatchDesign } from './batch-design'; import { generateId } from '../utils/id'; +import { ensureParentExists } from './element-tool-helpers'; export interface AddScrollRowV0Item { title: string; @@ -35,6 +36,7 @@ export interface AddScrollRowV0Params { export async function handleAddScrollRowV0( params: AddScrollRowV0Params, ): Promise>> { + await ensureParentExists(params); const gap = params.gap ?? 12; const cardWidth = params.card_width ?? defaultCardWidth(params.children_type); const wrapper = buildWrapperNode(params, gap, cardWidth); diff --git a/packages/pen-mcp/src/tools/element-tool-helpers.ts b/packages/pen-mcp/src/tools/element-tool-helpers.ts new file mode 100644 index 000000000..b76d3d5af --- /dev/null +++ b/packages/pen-mcp/src/tools/element-tool-helpers.ts @@ -0,0 +1,32 @@ +import { openDocument, resolveDocPath } from '../document-manager'; +import { findNodeInTree, getDocChildren } from '../utils/node-operations'; + +/** + * Validate that `parent_id` refers to an existing node before passing it to + * handleBatchDesign. batch_design's underlying `insertNodeInTree` silently + * returns the original tree when the parent is missing, producing a + * success-looking response (binding + nodeId) with an orphaned node that + * never lands on disk. Element tools (add_scroll_row_v0 / add_bottom_nav_v0 + * / add_activity_ring_v0) must fail fast with a clear error instead. + * + * Skips validation when parent_id is falsy (root-level insertion is always + * valid). Throws a descriptive Error otherwise. + */ +export async function ensureParentExists(params: { + parent_id?: string; + filePath?: string; + pageId?: string; +}): Promise { + if (!params.parent_id) return; + const fp = resolveDocPath(params.filePath); + const doc = await openDocument(fp); + const children = getDocChildren(doc, params.pageId); + const found = findNodeInTree(children, params.parent_id); + if (!found) { + throw new Error( + `parent_id "${params.parent_id}" not found in document${ + params.pageId ? ` (pageId=${params.pageId})` : '' + }. Pass a valid parent node id or omit parent_id for root-level insertion.`, + ); + } +}