Skip to content

Commit fbec07f

Browse files
feat(ai): capture remove_component's reverse for undo — position-preserving insert (#13272) (#13274)
Sub-B2 of #13221: remove_component undo-capture (gpt guardrail-2). Completes the create/remove set (Sub-B1 create #13264 merged). removeComponent server-stamps undoKind on its call_method destroy(true); the app-side InstanceService.callMethod captures the reverse BEFORE destroy (the component is gone after): snapshot parentId + child index (parent.indexOf) + a JSON-safe toJSON config -> reverse = call_method insert(index, config) on the parent -> position-preserving (re-insert at the original index, not append). Server-only marker (generic call_method forwards only {id,method,args}); fail-closed; undoReplay-suppressed. Tests (75 green incl. regression + the SERVER-side ComponentService.spec, which I'd missed running on Sub-B1): remove-capture; round-trip (remove -> undo -> re-dispatch insert(index,config) + tx consumed); generic-call_method non-capture (guardrail 1); buildRemoveReverse fail-closed branches; ComponentService dispatch now asserts the remove marker.
1 parent a74118b commit fbec07f

4 files changed

Lines changed: 197 additions & 7 deletions

File tree

ai/services/neural-link/ComponentService.mjs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,11 @@ class ComponentService extends Base {
210210
throw new Error('remove_component: `componentId` (the component to destroy) is required.');
211211
}
212212

213-
return await ConnectionService.call(sessionId, 'call_method', {id: componentId, method: 'destroy', args: [true]});
213+
// `undoKind` is a server-only capture marker (NOT in CallMethodRequest's schema): the app-side write-path
214+
// snapshots the component's parent + index + config BEFORE destroy so the `undo` tool can re-insert it at its
215+
// original position. The generic call_method service forwards only {id, method, args}, so a public caller
216+
// cannot inject this marker — generic call_method stays non-undoable.
217+
return await ConnectionService.call(sessionId, 'call_method', {id: componentId, method: 'destroy', args: [true], undoKind: 'remove_component'});
214218
}
215219
}
216220

src/ai/client/InstanceService.mjs

Lines changed: 71 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,70 @@ class InstanceService extends Service {
220220
}
221221
}
222222

223+
/**
224+
* Builds the reverse-op for a `remove_component` write — `remove⁻¹ = insert(index, config)` on the parent,
225+
* snapshotting the destroyed component's parent + tree index + a JSON-safe config BEFORE the destroy runs.
226+
* **Server-stamped only** (`undoKind === 'remove_component'`), canonical `destroy(true)` shape, writer identity
227+
* present, not an undo replay — else `null`, so {@link #recordUndo} no-ops (a generic `call_method` stays
228+
* non-undoable). Position-preserving: the reverse re-inserts at the original `index` (not an appending re-create),
229+
* so undo restores tree order. The config is a documented JSON-safe `toJSON` snapshot (serializable-config bound,
230+
* not full live-state fidelity); the data-not-code + payload-cap guards in {@link Neo.ai.TransactionService} bound it.
231+
* @param {Object} params
232+
* @param {Object|null} params.context The Bridge-stamped `{agentId, sessionId}` writer pair.
233+
* @param {String} params.id The component id being destroyed.
234+
* @param {String} params.method
235+
* @param {Array} params.args
236+
* @param {String} [params.undoKind] The server-stamped capture marker.
237+
* @param {Neo.component.Base} params.instance The live component, read BEFORE destroy.
238+
* @returns {Object|null} A reverse-record op, or `null` when the call is not a capturable remove.
239+
* @protected
240+
*/
241+
buildRemoveReverse({context, id, method, args, undoKind, instance}) {
242+
if (undoKind !== 'remove_component' || context?.undoReplay) {
243+
return null
244+
}
245+
246+
if (!context?.agentId || !context?.sessionId) {
247+
return null
248+
}
249+
250+
// canonical remove shape only — the server stamps destroy(true); any other shape is dropped (fail-closed)
251+
if (method !== 'destroy' || args.length !== 1 || args[0] !== true) {
252+
return null
253+
}
254+
255+
const
256+
parentId = instance.parentId,
257+
parent = parentId ? Neo.getComponent(parentId) : null;
258+
259+
if (!parent || typeof parent.indexOf !== 'function') {
260+
return null // a parentless / unresolvable target cannot be re-inserted — fail-closed
261+
}
262+
263+
const
264+
index = parent.indexOf(id),
265+
config = this.safeSerialize(typeof instance.toJSON === 'function' ? instance.toJSON() : null);
266+
267+
if (index < 0 || !config || typeof config !== 'object') {
268+
return null
269+
}
270+
271+
const targetSubtreePath = deriveSubtreePath(parentId, cid => Neo.getComponent(cid)?.parentId);
272+
273+
if (!targetSubtreePath) {
274+
return null
275+
}
276+
277+
return {
278+
sequenceId : `${id}:${++this.undoSequence}`,
279+
originWriter : {agentId: context.agentId, sessionId: context.sessionId},
280+
targetSubtreePath,
281+
forward : {tool: 'remove_component', args: {componentId: id}},
282+
reverse : {tool: 'call_method', args: {id: parentId, method: 'insert', args: [index, config]}},
283+
label : `remove ${id} from ${parentId}`
284+
}
285+
}
286+
223287
/**
224288
* Records a captured reverse-op as a single-op committed transaction on this heap's undo stack
225289
* ({@link Neo.ai.Client#transactionService}) — `begin` → `record` → `commit`, keyed on the writer's
@@ -337,13 +401,15 @@ class InstanceService extends Service {
337401

338402
this.assertWritable(context, id);
339403

404+
// remove_component reverse-capture must snapshot the component's re-creatable state BEFORE destroy runs (the
405+
// instance is gone afterwards); create_component's capture is post-call (the new child's id is the `add`
406+
// return). A server-stamped marker + the canonical shape gate each; a generic call_method + an undo replay
407+
// capture nothing. See {@link #buildRemoveReverse} / {@link #buildCreateReverse}.
408+
const removeOp = this.buildRemoveReverse({context, id, method, args, undoKind, instance});
409+
340410
const result = await scope[methodName].call(scope, ...args);
341411

342-
// create_component reverse-capture: a server-stamped create (the canonical `add(config)` dispatch) records
343-
// its inverse — destroy the new child — so an agent can undo a creation. The marker is server-only (the
344-
// server-side generic call_method forwards just {id, method, args}, so a public-injected `undoKind` is
345-
// stripped); a generic call_method + an undo replay are NOT captured. See {@link #buildCreateReverse}.
346-
this.recordUndo(context, this.buildCreateReverse({context, id, method, args, undoKind, result}));
412+
this.recordUndo(context, removeOp || this.buildCreateReverse({context, id, method, args, undoKind, result}));
347413

348414
return {result: this.safeSerialize(result)}
349415
}
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
import {setup} from '../../setup.mjs';
2+
3+
const appName = 'InstanceServiceRemoveUndoCaptureTest';
4+
5+
setup({
6+
neoConfig: {
7+
unitTestMode: true
8+
},
9+
appConfig: {
10+
name : appName,
11+
isMounted : () => true,
12+
vnodeInitialising: false
13+
}
14+
});
15+
16+
import {test, expect} from '@playwright/test';
17+
import Neo from '../../../../src/Neo.mjs';
18+
import * as core from '../../../../src/core/_export.mjs';
19+
import Component from '../../../../src/component/Base.mjs';
20+
import Container from '../../../../src/container/Base.mjs';
21+
import ComponentManager from '../../../../src/manager/Component.mjs'; // binds Neo.getComponent + registers components
22+
import InstanceManager from '../../../../src/manager/Instance.mjs'; // binds Neo.get + registers instances
23+
import InstanceService from '../../../../src/ai/client/InstanceService.mjs';
24+
import TransactionService from '../../../../src/ai/TransactionService.mjs';
25+
import WriteGuard from '../../../../src/ai/WriteGuard.mjs';
26+
27+
// `remove_component` forwards server-side as a server-stamped `call_method` `destroy(true)`; the app-side
28+
// InstanceService.callMethod captures the reverse (`insert(index, config)` on the parent) BEFORE destroy runs — the
29+
// component's parent/index/config must be snapshotted before it is gone. These tests isolate the capture wiring
30+
// (`indexOf`/`insert`/`destroy` are stubbed — their vdom path is Neo's, exercised elsewhere), asserting the
31+
// position-preserving reverse, the round-trip re-dispatch, the generic-call_method non-capture (gpt guardrail 1),
32+
// and the `buildRemoveReverse` fail-closed branches.
33+
34+
const ID = {agentId: 'agent-a', sessionId: 'sess-a'};
35+
let seq = 0; // unique ids per test (Date/random are unavailable; a stubbed destroy won't unregister)
36+
37+
test.describe('Neo.ai.client.InstanceService — remove_component undo capture', () => {
38+
let parent, child, parentId, childId, service, transactionService, dispatched;
39+
40+
test.beforeEach(() => {
41+
seq++;
42+
parentId = `rm-parent-${seq}`;
43+
childId = `rm-child-${seq}`;
44+
45+
transactionService = Neo.create(TransactionService);
46+
dispatched = [];
47+
48+
const client = {
49+
transactionService,
50+
writeGuard : Neo.create(WriteGuard),
51+
// mirrors Neo.ai.Client#handleRequest; records the undo re-dispatch + routes it back to the service
52+
handleRequest: (method, params, ctx) => {
53+
dispatched.push({method, params});
54+
return service[Neo.snakeToCamel(method)]?.(params, ctx)
55+
}
56+
};
57+
58+
service = Neo.create(InstanceService, {client});
59+
parent = Neo.create(Container, {appName, id: parentId, items: []});
60+
child = Neo.create(Component, {appName, id: childId, width: 120});
61+
62+
child.parentId = parentId;
63+
child.toJSON = () => ({ntype: 'component', id: childId, width: 120}); // stub: a small JSON-safe config — a real toJSON can exceed record()'s payload cap / carry non-serializable refs (the documented serializable-config bound)
64+
parent.indexOf = () => 2; // stub: the child's tree position (real indexOf needs it mounted in items)
65+
parent.insert = () => child; // stub: the re-insert (its vdom path is Neo's; we assert the re-dispatch args)
66+
child.destroy = () => {} // stub: skip the unitTestMode vdom teardown (capture happens before destroy)
67+
});
68+
69+
test.afterEach(() => {
70+
!parent.isDestroyed && parent.destroy()
71+
});
72+
73+
test('captures remove\'s inverse — insert(index, config) on the parent — snapshotted before destroy', async () => {
74+
await service.callMethod({id: childId, method: 'destroy', args: [true], undoKind: 'remove_component'}, ID);
75+
76+
const stack = transactionService.stackOf({id: ID});
77+
expect(stack.committed).toHaveLength(1);
78+
79+
const op = stack.committed[0].ops[0];
80+
expect(op.forward).toEqual({tool: 'remove_component', args: {componentId: childId}});
81+
expect(op.reverse.tool).toBe('call_method');
82+
expect(op.reverse.args.id).toBe(parentId);
83+
expect(op.reverse.args.method).toBe('insert');
84+
expect(op.reverse.args.args[0]).toBe(2); // the captured index (position-preserving)
85+
expect(op.reverse.args.args[1]).toMatchObject({id: childId}); // the captured config (toJSON snapshot)
86+
expect(op.originWriter).toEqual(ID)
87+
});
88+
89+
test('round-trip: a captured remove → undo → re-dispatches insert(index, config) + consumes the tx', async () => {
90+
await service.callMethod({id: childId, method: 'destroy', args: [true], undoKind: 'remove_component'}, ID);
91+
expect(transactionService.stackOf({id: ID}).committed).toHaveLength(1);
92+
93+
const result = await service.undo({}, ID);
94+
95+
expect(result.undone).toBe(true);
96+
const insertCall = dispatched.find(d => d.params?.method === 'insert');
97+
expect(insertCall).toBeTruthy();
98+
expect(insertCall.params.id).toBe(parentId);
99+
expect(insertCall.params.args[0]).toBe(2); // re-inserted at the original index
100+
expect(transactionService.stackOf({id: ID}).committed).toHaveLength(0) // tx consumed
101+
});
102+
103+
test('a generic call_method destroy (NO server-stamped marker) is NOT captured — generic call_method stays non-undoable', async () => {
104+
await service.callMethod({id: childId, method: 'destroy', args: [true]}, ID); // no undoKind
105+
106+
expect(transactionService.stackOf({id: ID}).committed).toHaveLength(0)
107+
});
108+
109+
test('buildRemoveReverse is fail-closed — only a marked, canonical, attributed, non-replay remove captures', () => {
110+
const base = {context: ID, id: childId, method: 'destroy', args: [true], undoKind: 'remove_component', instance: child};
111+
112+
expect(service.buildRemoveReverse(base)).not.toBeNull(); // happy path
113+
expect(service.buildRemoveReverse({...base, undoKind: undefined})).toBeNull(); // no marker
114+
expect(service.buildRemoveReverse({...base, context: {...ID, undoReplay: true}})).toBeNull(); // undo replay
115+
expect(service.buildRemoveReverse({...base, context: null})).toBeNull(); // no writer identity
116+
expect(service.buildRemoveReverse({...base, method: 'hide'})).toBeNull(); // non-canonical method
117+
expect(service.buildRemoveReverse({...base, args: [false]})).toBeNull(); // non-canonical arg (not destroy(true))
118+
expect(service.buildRemoveReverse({...base, instance: {parentId: null, toJSON: () => ({})}})).toBeNull() // unresolvable parent
119+
})
120+
});

test/playwright/unit/ai/services/neural-link/ComponentService.spec.mjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ test.describe('Neo.ai.services.neural-link.ComponentService — createComponent
126126
expect(calls[0]).toEqual({
127127
sessionId: 's1',
128128
op : 'call_method',
129-
payload : {id: 'dialog-1', method: 'destroy', args: [true]}
129+
payload : {id: 'dialog-1', method: 'destroy', args: [true], undoKind: 'remove_component'} // server-stamped undo-capture marker
130130
});
131131
});
132132
});

0 commit comments

Comments
 (0)