SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(mcp): post-insert must verify parent location, not just presence

Browse source 
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.
This commit is contained in:
Fini 2026-04-19 18:12:43 +08:00
parent 5847b50d19
commit ea5057380c
2 changed files with 68 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

47
packages/pen-mcp/src/__tests__/element-tools-contract.test.ts
View file

@ -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/);
});
});

22
packages/pen-mcp/src/tools/element-tool-helpers.ts
View file

@ -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;
}