From ea5057380ccbf9fbffa308f708a633be9f9f5c1e Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 19 Apr 2026 18:12:43 +0800 Subject: [PATCH] fix(mcp): post-insert must verify parent location, not just presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-hook #4: the prior post-insert check (e797584) verified that the inserted nodeId is findable anywhere in the tree, but did not verify it landed under the REQUESTED parent. Silent wrong-parent inserts are still possible if batch_design's resolveRef quote-strip produces a literal that matches a DIFFERENT node than the one ensureParentExists validated. Concrete example: doc has nodes with ids `A"B` (3 chars: A, ", B) and `A\"B` (4 chars: A, \, ", B). User passes parent_id='A"B'. ensureParentExists does a raw-string `===` match and finds the first node. insertElementTree does JSON.stringify → `"A\"B"` in DSL source. batch_design's parseInsertArgs → resolveRef → `/^"|"$/g` quote-strip → literal `A\"B` (4 chars). insertNodeInTree matches the DECOY and inserts under it. Prior post-check: node is in tree → passes. Actual: wrong parent. Fix: when args.parent_id is provided, post-check also walks findParentInTree(postChildren, insertedId) and confirms the resolved parent id === the requested parent_id. Mismatch → throw with clear diagnostic. Regression test: element-tools-contract.test.ts adds the A"B / A\"B decoy scenario and asserts the tool throws (not silent success). 88/88 pen-mcp tests pass (+1 new wrong-parent guard). format + tsc green. Bundle rebuilt. --- .../__tests__/element-tools-contract.test.ts | 47 +++++++++++++++++++ .../pen-mcp/src/tools/element-tool-helpers.ts | 22 ++++++++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/packages/pen-mcp/src/__tests__/element-tools-contract.test.ts b/packages/pen-mcp/src/__tests__/element-tools-contract.test.ts index ea44388e1..ed53c7800 100644 --- a/packages/pen-mcp/src/__tests__/element-tools-contract.test.ts +++ b/packages/pen-mcp/src/__tests__/element-tools-contract.test.ts @@ -152,4 +152,51 @@ describe('element tools — silent no-op guards (P0.1 + post-insert check)', () const after = await readFile(fp, 'utf-8'); expect(after).toBe(before); }); + + it('throws when DSL-resolved parent_id collides with a different node (wrong-parent insert)', async () => { + // Seed the document with TWO frames: + // - intended parent, id = `A"B` (3 chars: A, ", B) — the user's target + // - decoy parent, id = `A\"B` (4 chars: A, \, ", B) — happens to + // be what batch_design's resolveRef produces after quote-strip on + // our JSON.stringify output for 'A"B' + // Expected: ensureParentExists finds the intended parent (raw string + // match ✓), but insertNodeInTree resolves to the decoy (literal match + // on the 4-char id). Without parent-location verification, the node + // would silently land under the wrong parent. + const fp = join(TMP, 'quoted-parent.op'); + await writeFile( + fp, + JSON.stringify({ + version: '1.0.0', + children: [ + { + id: 'A"B', + type: 'frame', + name: 'Intended Parent', + width: 1200, + height: 0, + layout: 'vertical', + children: [], + }, + { + id: 'A\\"B', + type: 'frame', + name: 'Decoy Parent', + width: 1200, + height: 0, + layout: 'vertical', + children: [], + }, + ], + }), + 'utf-8', + ); + await expect( + handleAddBottomNavV0({ + filePath: fp, + parent_id: 'A"B', + items: [{ title: 'Home', icon: 'home' }], + }), + ).rejects.toThrow(/wrong parent|not present/); + }); }); diff --git a/packages/pen-mcp/src/tools/element-tool-helpers.ts b/packages/pen-mcp/src/tools/element-tool-helpers.ts index ab8314f93..6c8c2e3fa 100644 --- a/packages/pen-mcp/src/tools/element-tool-helpers.ts +++ b/packages/pen-mcp/src/tools/element-tool-helpers.ts @@ -1,5 +1,5 @@ import { openDocument, resolveDocPath } from '../document-manager'; -import { findNodeInTree, getDocChildren } from '../utils/node-operations'; +import { findNodeInTree, findParentInTree, getDocChildren } from '../utils/node-operations'; import { generateId } from '../utils/id'; import { handleBatchDesign } from './batch-design'; @@ -147,5 +147,25 @@ export async function insertElementTree(args: { `parent_id=${JSON.stringify(args.parent_id)}, pageId=${JSON.stringify(args.pageId)}.`, ); } + // Parent-location verification: node exists in tree but may have landed + // under the wrong parent. Can happen if batch_design's resolveRef + // quote-strip produces a literal that matches a DIFFERENT node than the + // one pre-check validated. Example: doc has both `A"B` (user intent) + // AND `A\"B` (literal 4-char id with backslash); after JSON.stringify + // + quote-strip, parser resolves to `A\"B` and inserts under it. + // ensureParentExists and the "landed in tree" check both pass, but + // the insert went to the wrong place. + if (args.parent_id) { + const actualParent = findParentInTree(postChildren, insertedId); + const actualParentId = actualParent?.id ?? null; + if (actualParentId !== args.parent_id) { + throw new Error( + `Element tool insert landed under the wrong parent: expected ` + + `${JSON.stringify(args.parent_id)}, got ${JSON.stringify(actualParentId ?? 'root')}. ` + + `This is typically a DSL-parser escape mismatch where the resolved parent id ` + + `happens to collide with a different node's literal id.`, + ); + } + } return result; }