fix(mcp): assign ids to every node in add_scroll_row_v0 subtree

Codex stop-hook review: child nodes were saved without ids. batch_design
only assigns an id to the top-level inserted node — nested children
(inner row, cards, texts, icon_fonts) come through the DSL unchanged,
which breaks any later tree operation that resolves by id (update /
delete / move / post-processing / spatial index lookup).

Fix: assignIdsRecursively walks the wrapper subtree before serializing
to DSL and stamps every node with generateId(). batch_design's own
overwrite of the top-level id is harmless.

Test: 4 new id-coverage tests (one per children_type + uniqueness
across a 3-card tree). Total 17 tests in add-scroll-row-v0, 74 in
pen-mcp suite. format + tsc green.
This commit is contained in:
Fini 2026-04-19 15:39:42 +08:00
parent b8744fbda3
commit bc2a9a38b1
2 changed files with 111 additions and 0 deletions

View file

@ -243,6 +243,99 @@ describe('add_scroll_row_v0 — overrides', () => {
});
});
describe('add_scroll_row_v0 — id assignment (every node must have a valid id)', () => {
function collectIds(
node: Record<string, unknown>,
out: { total: number; missing: string[] },
path = 'root',
): void {
const id = node.id;
if (typeof id !== 'string' || id.length === 0) {
out.missing.push(`${path} (type=${String(node.type)} name=${String(node.name)})`);
}
out.total += 1;
const children = node.children;
if (Array.isArray(children)) {
children.forEach((c, i) => {
if (c && typeof c === 'object') {
collectIds(c as Record<string, unknown>, out, `${path}/${i}`);
}
});
}
}
it('every node in a card row (wrapper+row+cards+grandchildren) has a non-empty id', async () => {
const fp = await fresh('cards.op');
await handleAddScrollRowV0({
filePath: fp,
children_type: 'card',
items: [{ title: 'A', subtitle: 'x', icon: 'i' }, { title: 'B' }],
});
const wrapper = getRoot(await readDoc(fp));
const collected = { total: 0, missing: [] as string[] };
collectIds(wrapper, collected);
expect(collected.missing).toEqual([]);
// wrapper + row + 2 cards + (3 + 1) card children = 8
expect(collected.total).toBe(8);
});
it('every node in a metric_tile row has a non-empty id', async () => {
const fp = await fresh('metrics.op');
await handleAddScrollRowV0({
filePath: fp,
children_type: 'metric_tile',
items: [
{ title: 'Steps', subtitle: '8,432' },
{ title: 'Kcal', subtitle: '512', icon: 'flame' },
],
});
const wrapper = getRoot(await readDoc(fp));
const collected = { total: 0, missing: [] as string[] };
collectIds(wrapper, collected);
expect(collected.missing).toEqual([]);
});
it('every node in a nav_item row has a non-empty id', async () => {
const fp = await fresh('nav.op');
await handleAddScrollRowV0({
filePath: fp,
children_type: 'nav_item',
items: [
{ title: 'Home', icon: 'home' },
{ title: 'Search', icon: 'search' },
],
});
const wrapper = getRoot(await readDoc(fp));
const collected = { total: 0, missing: [] as string[] };
collectIds(wrapper, collected);
expect(collected.missing).toEqual([]);
});
it('all ids in the tree are unique', async () => {
const fp = await fresh('cards.op');
await handleAddScrollRowV0({
filePath: fp,
children_type: 'card',
items: [
{ title: 'A', subtitle: 'x', icon: 'i' },
{ title: 'B', subtitle: 'y', icon: 'j' },
{ title: 'C' },
],
});
const wrapper = getRoot(await readDoc(fp));
const ids: string[] = [];
function collect(n: Record<string, unknown>): void {
if (typeof n.id === 'string') ids.push(n.id);
const kids = n.children;
if (Array.isArray(kids)) {
for (const k of kids) if (k && typeof k === 'object') collect(k as Record<string, unknown>);
}
}
collect(wrapper);
expect(new Set(ids).size).toBe(ids.length);
});
});
describe('add_scroll_row_v0 — persistence', () => {
it('result.nodeCount reflects full node tree (wrapper+row+cards+children)', async () => {
const fp = await fresh('persist.op');

View file

@ -1,4 +1,5 @@
import { handleBatchDesign } from './batch-design';
import { generateId } from '../utils/id';
export interface AddScrollRowV0Item {
title: string;
@ -37,6 +38,11 @@ export async function handleAddScrollRowV0(
const gap = params.gap ?? 12;
const cardWidth = params.card_width ?? defaultCardWidth(params.children_type);
const wrapper = buildWrapperNode(params, gap, cardWidth);
// batch_design only assigns an id to the top-level inserted node; nested
// children come through untouched. Without ids on nested nodes, later
// tree traversal / update / delete / move all fail. Assign ids to the
// entire subtree here (batch_design harmlessly overwrites the top id).
assignIdsRecursively(wrapper);
const parentRef = params.parent_id ? `"${params.parent_id}"` : 'null';
const dsl = `row=I(${parentRef}, ${JSON.stringify(wrapper)})`;
return handleBatchDesign({
@ -47,6 +53,18 @@ export async function handleAddScrollRowV0(
});
}
function assignIdsRecursively(node: Record<string, unknown>): void {
if (typeof node.id !== 'string') node.id = generateId();
const children = node.children;
if (Array.isArray(children)) {
for (const child of children) {
if (child && typeof child === 'object') {
assignIdsRecursively(child as Record<string, unknown>);
}
}
}
}
function defaultCardWidth(kind: AddScrollRowV0ChildrenType): number {
switch (kind) {
case 'card':