fix(mcp): fail fast on invalid parent_id in element tools
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.
This commit is contained in:
parent
0799ad4843
commit
6b0feb3ba5
|
|
@ -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/);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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/);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>[] }> | undefined;
|
||||
const topLevel = doc['children'] as Record<string, unknown>[] | undefined;
|
||||
const topChildren = topLevel ?? pages?.[0]?.children ?? [];
|
||||
const container = topChildren[0];
|
||||
expect(container.id).toBe('container-1');
|
||||
const wrapper = (container.children as Record<string, unknown>[])[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');
|
||||
|
|
|
|||
|
|
@ -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'],
|
||||
|
|
|
|||
|
|
@ -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<Awaited<ReturnType<typeof handleBatchDesign>>> {
|
||||
await ensureParentExists(params);
|
||||
const size = params.size ?? 80;
|
||||
const thickness = params.thickness ?? 8;
|
||||
const ringColor = params.ring_color ?? '#000000';
|
||||
|
|
|
|||
|
|
@ -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<Awaited<ReturnType<typeof handleBatchDesign>>> {
|
||||
await ensureParentExists(params);
|
||||
const height = params.height ?? 62;
|
||||
const nav = buildNav(params, height);
|
||||
assignIdsRecursively(nav);
|
||||
|
|
|
|||
|
|
@ -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<Awaited<ReturnType<typeof handleBatchDesign>>> {
|
||||
await ensureParentExists(params);
|
||||
const gap = params.gap ?? 12;
|
||||
const cardWidth = params.card_width ?? defaultCardWidth(params.children_type);
|
||||
const wrapper = buildWrapperNode(params, gap, cardWidth);
|
||||
|
|
|
|||
32
packages/pen-mcp/src/tools/element-tool-helpers.ts
Normal file
32
packages/pen-mcp/src/tools/element-tool-helpers.ts
Normal file
|
|
@ -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<void> {
|
||||
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.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue