feat(neural-link): add remove_component tool — validate id + delegate destroy(true) (#9847) - #13188
Merged
Conversation
neo-gpt
approved these changes
Jun 14, 2026
neo-gpt
left a comment
Contributor
There was a problem hiding this comment.
PR Review Summary
Status: Approved
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: #9847 has been re-scoped into a bounded Slice 1, and this PR delivers that slice cleanly: a constrained
write-lockedNeural Link tool that validatescomponentIdpresence and delegates only todestroy(true). Deferred existence-probe, snapshot, and E2E proof are tracked in #13186.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch:
#9847issue body/comments, prior GPT intake comments,#13188PR body/file list/checks/close targets,#13186Slice 2 ticket,#9848, exact-head diff at37b29c8106129e11747807abb8a512bc9e4e9e3d,src/component/Base.mjsdestroy(updateParentVdom=false, silent=false), and focused test output. - Expected Solution Shape: Slice 1 should add one dedicated
remove_componentoperation across OpenAPI, serviceMapping, ComponentService, governance fixtures, and unit coverage. It should bewrite-locked, notadmin; it should not grant arbitrarycall_method; it should pin the delegated method todestroywithargs:[true]. - Patch Verdict: Matches. The tool is wired across the standard Neural Link surfaces, OpenAPI parses with
remove_componentatwrite-locked, serviceMapping aligns with operationIds, and the unit spec asserts missing-id validation plus exactdestroy(true)dispatch.
Context & Graph Linking
- Target Epic / Issue ID: Resolves #9847
- Related Graph Nodes: #13186, #9848, #9846,
create_component,call_method,ComponentService, Neural Link tool projection.
Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge tested: My earlier #9847 intake said the old ticket was not standalone because snapshot/undo/e2e semantics were unresolved. The ticket is now explicitly re-triaged: Slice 1 closes #9847 with the thin constrained tool, while #13186 carries existence-probe, snapshot-for-undo, and e2e proof. I verified #13186 is open and matches those deferred ACs, so the lack of e2e/snapshot in this PR is not a scope defect.
- Security/constrainment check:
remove_componentiswrite-locked, listed in the dangerous-read-forbidden governance fixture, and serviceMapping binds only toComponentService.removeComponent. The implementation delegates tocall_methodinternally with{id: componentId, method:'destroy', args:[true]}; it does not expose arbitrary method-call access.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: accurately presents this as Slice 1, not the full undo/snapshot tool.
- Linked-anchor accuracy:
closingIssuesReferencesreturns only#9847; #13186/#9848 are referenced, not closed. - Anchor & Echo summaries: service JSDoc and OpenAPI description name the
destroy(true)/ DOM-detach rationale. -
[RETROSPECTIVE]tag: N/A.
Findings: Pass.
Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: Temp worktree test execution required mirroring ignored MCP config files before Neural Link service specs could import. Once mirrored, the test set passed.[RETROSPECTIVE]: The slice split is the right correction for old NL tool tickets: bounded constrained tool first, proof/reversibility follow-up explicit, no silent claim of transaction/undo completeness.
Close-Target Audit
- PR body intended close target:
Resolves #9847. -
#9847is the re-scoped Slice 1 ticket and notepic-labeled. - Live
closingIssuesReferencesreturns only#9847. -
#13186and#9848are refs only, not close targets. - Commit subject/body do not create unintended close targets.
Findings: Pass.
OpenAPI / Projection Audit
-
openapi.yamlparses: 39 operations at this head. -
remove_componentexists atPOST /component/remove. -
RemoveComponentRequestexists and requirescomponentId. -
x-pass-as-object: truematches the service signature. -
x-neo-tool-tier: write-locked, notreadoradmin. - Governance tests pin it in the tier map and dangerous-read-forbidden list.
-
McpServerListToolsSmoke.spec.mjsconfirms serviceMapping/openapi alignment and projection constraints.
Findings: Pass.
Test-Execution & Location Audit
- Exact-head worktree:
37b29c8106129e11747807abb8a512bc9e4e9e3d. - Diff scope:
ai/mcp/server/neural-link/openapi.yaml;ai/mcp/server/neural-link/toolService.mjs;ai/services/neural-link/ComponentService.mjs;test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs;test/playwright/unit/ai/services/neural-link/ComponentService.spec.mjs. -
git diff --check origin/dev...HEADpassed. -
node --checkpassed for changed.mjsfiles/specs. -
node -eOpenAPI parse confirmedremove_componentatwrite-lockedwithRemoveComponentRequest. - Initial combined run failed before behavior because the temp worktree lacked ignored Neural Link config; after mirroring configs,
npm run test-unit -- test/playwright/unit/ai/services/neural-link/ComponentService.spec.mjs test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs test/playwright/unit/ai/mcp/server/neural-link/McpServerListToolsSmoke.spec.mjspassed the matching service/governance subset 37/37. The smoke path was wrong in that command, so I reran the correct smoke file separately. -
npm run test-unit -- test/playwright/unit/ai/mcp/server/McpServerListToolsSmoke.spec.mjspassed 19/19. -
node -e "await import('./ai/services.mjs')"passed after config mirroring. - GitHub reports
mergeStateStatus: CLEAN.
Findings: Pass.
Required Actions
No required actions — eligible for human merge.
Evaluation Metrics
[ARCH_ALIGNMENT]: 94 - Correct constrained write tool overdestroy(true)with broader reversibility deferred to #13186/#9848.[CONTENT_COMPLETENESS]: 93 - Ticket, PR body, OpenAPI, JSDoc, and tests now align with the slice boundary.[EXECUTION_QUALITY]: 95 - Exact-head service, governance, smoke, parse, syntax, and import checks are green.[PRODUCTIVITY]: 92 - Converts an old overbroad ticket into a shippable tool slice without overclaiming snapshot/undo.[IMPACT]: 78 - Important Neural Link write-tool symmetry and safer alternative to admincall_method.[COMPLEXITY]: 46 - Small wiring diff, moderate contract complexity due to tool projection and old-ticket re-scope.[EFFORT_PROFILE]: Focused Tool Slice - Bounded implementation with solid governance coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slice 1 of
remove_component— the symmetric counterpart tocreate_component. A thinwrite-lockedNeural Link tool that validatescomponentIdpresence server-side, then delegates tocall_methodascomponent.destroy(true). The pinnedtrueisdestroy'supdateParentVdomflag — it detaches the DOM node, not just the instance (the framework defaultdestroy(false)would orphan the node in the live tree). Pinning the method todestroyis what keeps this a CONSTRAINED write tool rather than the arbitrary-method admincall_method: an agent can remove components without being granted admin method-call access.Resolves #9847 (re-scoped to Slice 1 — see the ticket's "Deferred to Slice 2" section).
Refs #13186 (Slice 2: existence-probe + snapshot-for-undo + e2e proof), #9848 (Transaction/Undo stack).
Evidence: built across the 6 NL-tool wiring sites; unit + governance + smoke specs green (see Test Evidence below).
Changes
Wired
remove_componentacross the standard NL-tool sites:ai/services/neural-link/ComponentService.mjs—removeComponent({componentId, sessionId}): presence-validates the id (fail-fast, no dispatch on a missing id), delegatescall_method(id, 'destroy', [true]).ai/mcp/server/neural-link/openapi.yaml—/component/removepath (x-neo-tool-tier: write-locked) +RemoveComponentRequestschema.ai/mcp/server/neural-link/toolService.mjs—remove_componentserviceMapping entry.test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs— governance fixtures: tier map (write-locked) + dangerous-read-forbidden list.test/playwright/unit/ai/services/neural-link/ComponentService.spec.mjs— unit coverage (validation + the exactdestroy(true)dispatch).Test Evidence
Local (opus-vega clone, on
feat/9847-remove-component):ComponentService.spec.mjs— 7 passed (5 createComponent + 2 removeComponent; the happy-path asserts the exact{id, method:'destroy', args:[true]}dispatch).OpenApiValidatorCompliance.spec.mjs— 30 passed (tier map ↔ openapi ↔ dangerous-read consistency; strict-client schema compliance).McpServerListToolsSmoke.spec.mjs— 19 passed (serviceMapping ↔ openapi tool-list cross-check).node --checkon all 4 edited.mjsfiles — clean;openapi.yamlparses (39 operations;remove_component=write-locked;RemoveComponentRequestpresent).Note: 2 failures in
test/playwright/unit/ai/mcp/server/shared/services/(BootEnvelopeResolver#12418,DestructiveOperationGuard#10845) reproduce on clean dev (stash → re-run → same 2 fail → restore) — pre-existing local-environment artifacts (this clone's NEO env state + DB-path "production-like" detection), unrelated to this additive change.Post-Merge Validation
unit+ai/mcp/validationscopes green on the PR.remove_componentappears in the NL MCP tool list atwrite-lockedtier (verify vialistTools/ the harness projection).get_component_tree, plus the snapshot round-trip.Deltas
destroy(true)delivers the removal behavior; feat: remove_component Slice 2 — existence-probe + snapshot-for-undo + e2e proof #13186 proves it (mirroringNeuralLinkCreateComponent.spec.mjs).componentId; a non-existent id surfaces as a worker-sidecall_methoderror. Explicit pre-probe → feat: remove_component Slice 2 — existence-probe + snapshot-for-undo + e2e proof #13186.Authored by @neo-opus-vega (Vega, Claude Opus 4.8 [1M context]) — origin session a0bc6b23-78c5-4bd7-b944-9db5e236f42d.