diff --git a/backend/domain/graph.py b/backend/domain/graph.py index 3d391040..49595339 100644 --- a/backend/domain/graph.py +++ b/backend/domain/graph.py @@ -547,8 +547,51 @@ def add_chat_node( self.nodes[node_id] = node if parent_id is not None: self.connect(parent_id, node_id) + else: + self.adopt_pending_system_prompt(node_id) return node + def adopt_pending_system_prompt(self, root_id: str) -> SceneEdge | None: + """Connects an unattached system-prompt note to a new branch root. + + plugins/system_prompt/ can create its note with nothing selected, so + that a prompt can be written BEFORE the conversation it governs - + the natural order, and the one the old "select a node first" rule + made impossible without a wasted send. Such a note starts with no + edges, and AgentDispatcher._resolve_branch_system_prompt only ever + looks for a note -> root edge, so the note would stay silently inert + until something drew that edge. This is that something: the first + parentless chat node - a new branch root - adopts it. + + Deliberately narrow. It fires only for a node created with no + parent, only when exactly one unattached system-prompt note exists, + and only while that root has no system prompt of its own; anything + more ambiguous is left alone rather than guessed at. Returns the + edge it created, or None when it did nothing.""" + root = self.nodes.get(root_id) + if root is None: + return None + sources = {edge.source for edge in self.edges.values()} + pending = [ + node + for node in self.nodes.values() + if node.kind == "note" + and getattr(node.state, "is_system_prompt", False) + and node.id not in sources + ] + if len(pending) != 1: + return None + already_attached = any( + edge.target == root_id + and (source := self.nodes.get(edge.source)) is not None + and source.kind == "note" + and getattr(source.state, "is_system_prompt", False) + for edge in self.edges.values() + ) + if already_attached: + return None + return self.connect(pending[0].id, root_id) + def add_code_node( self, x: float, @@ -621,7 +664,7 @@ def add_document_node( see DocumentState's own docstring (backend/domain/node_states.py) for those exact rules. """ - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" # Mirrors DocumentNode.__init__'s `(attachment_kind or @@ -648,7 +691,8 @@ def add_document_node( ), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def add_thinking_node( @@ -674,7 +718,7 @@ def add_thinking_node( there is no delete_thinking_node; deletion goes entirely through the existing generic remove_nodes. """ - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" title = str(thinking_text)[:THINKING_TITLE_PREVIEW_LENGTH] or "Thinking" @@ -687,7 +731,8 @@ def add_thinking_node( content=str(thinking_text), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def add_html_node( @@ -716,7 +761,7 @@ def add_html_node( thinking): there is no delete_html_node; deletion goes entirely through the existing generic remove_nodes. """ - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" title = str(html_content)[:HTML_TITLE_PREVIEW_LENGTH] or "HTML" @@ -730,7 +775,8 @@ def add_html_node( state=HtmlState(), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def set_html_splitter_state(self, node_id: str, value: float) -> None: @@ -788,7 +834,7 @@ def add_image_node( additionally evicts this node's image_assets entry so bytes never outlive the node (see remove_nodes). """ - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" asset_id = f"img{uuid.uuid4().hex}" @@ -804,7 +850,8 @@ def add_image_node( state=ImageState(image_asset_id=asset_id), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def get_image_asset(self, asset_id: str) -> tuple[bytes, str] | None: @@ -838,7 +885,7 @@ def add_conversation_node(self, x: float, y: float, parent_id: str) -> SceneNode thinking/html/image): there is no delete_conversation_node; deletion goes entirely through the existing generic remove_nodes. """ - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -849,7 +896,8 @@ def add_conversation_node(self, x: float, y: float, parent_id: str) -> SceneNode kind="conversation", ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def append_conversation_user_message(self, node_id: str, text: str) -> SceneNode: @@ -915,7 +963,7 @@ def add_web_research_node(self, x: float, y: float, parent_id: str) -> SceneNode is no meaningful single preview string before a query has ever been run). Content starts empty; the query text only lands once start_web_research_run is called.""" - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -927,7 +975,8 @@ def add_web_research_node(self, x: float, y: float, parent_id: str) -> SceneNode state=WebResearchState(), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def start_web_research_run(self, node_id: str, query: str) -> SceneNode: @@ -1003,7 +1052,7 @@ def add_artifact_node(self, x: float, y: float, parent_id: str) -> SceneNode: document has ever been drafted). artifact_content starts empty; the document text only lands once complete_artifact_generation is called.""" - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -1015,7 +1064,8 @@ def add_artifact_node(self, x: float, y: float, parent_id: str) -> SceneNode: state=ArtifactState(), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def append_artifact_user_message(self, node_id: str, text: str) -> SceneNode: @@ -1072,7 +1122,7 @@ def add_gitlink_node(self, x: float, y: float, parent_id: str) -> SceneNode: invalid_parent_message for Gitlink, there is no unparented/root form in the domain model). Title is always the fixed literal "Gitlink" (mirrors conversation/web_research/artifact's own fixed titles).""" - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -1084,7 +1134,8 @@ def add_gitlink_node(self, x: float, y: float, parent_id: str) -> SceneNode: state=GitlinkState(), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def set_gitlink_local_root(self, node_id: str, local_root: str) -> SceneNode: @@ -1319,7 +1370,7 @@ def add_code_sandbox_node(self, x: float, y: float, parent_id: str) -> SceneNode sandbox directory name (VirtualEnvSandbox re-sanitizes it again on its own side, but a short, already-safe id keeps the on-disk path short and human-scannable).""" - if parent_id not in self.nodes: + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -1331,7 +1382,8 @@ def add_code_sandbox_node(self, x: float, y: float, parent_id: str) -> SceneNode state=CodeSandboxState(code_sandbox_sandbox_id=uuid.uuid4().hex[:12]), ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node def set_web_research_retain_to_knowledge(self, node_id: str, retain: bool) -> SceneNode: @@ -1942,7 +1994,7 @@ def remove_nodes(self, node_ids: list[str]) -> None: # -- ADR-014 stage 14.1: Plugin SDK node-creation primitive ------------- def add_plugin_node( - self, kind: str, x: float, y: float, parent_id: str, *, + self, kind: str, x: float, y: float, parent_id: str | None, *, title: str = "", content: str = "", state: NodeState | None = None, ) -> SceneNode: """Generic node-creation primitive for ADR-014's Plugin SDK @@ -1952,9 +2004,15 @@ def add_plugin_node( parent. `kind` arrives here ALREADY namespaced as f"{plugin_id}.{local_kind}" (backend/plugin_sdk.py's HostContext. register_node_kind) - this method itself does no namespacing or - validation of its own, same "trust the caller, one required-parent - posture" contract as add_web_research_node/add_gitlink_node/etc.""" - if parent_id not in self.nodes: + validation of its own, same "trust the caller" contract as + add_web_research_node/add_gitlink_node/etc. + + `parent_id=None` creates the node UNCONNECTED, at the given x/y. + That is the one deliberate difference from its add_X_node siblings, + and it is what a plugin registered with requires_parent=False needs: + such a plugin is creatable on an empty canvas, where by definition + there is no parent to attach to.""" + if parent_id is not None and parent_id not in self.nodes: raise SceneError(f"unknown parent node: {parent_id}") node_id = f"n{next(self._counter)}" node = SceneNode( @@ -1962,7 +2020,8 @@ def add_plugin_node( title=str(title) or kind, kind=str(kind), content=str(content), state=state, ) self.nodes[node_id] = node - self.connect(parent_id, node_id) + if parent_id is not None: + self.connect(parent_id, node_id) return node # -- edges ------------------------------------------------------------- diff --git a/backend/plugin_sdk.py b/backend/plugin_sdk.py index 01e1e200..fe690fde 100644 --- a/backend/plugin_sdk.py +++ b/backend/plugin_sdk.py @@ -340,6 +340,16 @@ class PluginRunContext: plugin_id: str notifications: NotificationState + # Where a PARENTLESS node should be created, in scene coordinates. The + # picker sends the viewport's own center with every executePlugin call, + # so a plugin registered with requires_parent=False has a real, + # host-decided place to spawn - the gap that made requires_parent=False + # unrepresentable in v1 and, with it, forced every plugin (System Prompt + # included) to demand a pre-existing selected node before it could run. + # None only when a host constructs a context outside the picker path; + # parentless creation falls back to the origin in that case. + spawn_x: float | None = None + spawn_y: float | None = None @dataclass(frozen=True) @@ -347,7 +357,9 @@ class NodeKindSpec: plugin_id: str kind: str # ALREADY namespaced: f"{plugin_id}.{local_kind}" factory: NodeFactory - requires_parent: bool # always True in v1 + # False = creatable with nothing selected, spawned at the picker's + # reported viewport center (PluginRunContext.spawn_x/spawn_y). + requires_parent: bool # ADR-014 stage 14.2: OPTIONAL persistence/wire hooks - see # HostContext.register_node_kind's own docstring for the full contract. # A plugin that passes neither still gets its node's universal title/ @@ -449,6 +461,10 @@ class BuiltinActionSpec: description: str category: str handler: BuiltinActionHandler + # Mirrors PluginNodeKindSpec.requires_parent: False lets the picker run + # this action with nothing selected. The handler still owns its own + # validation - the host only stops gating the call on a parent id. + requires_parent: bool = True # SECURITY-FIX: register_builtin_plugin (below) attaches a picker action that @@ -501,10 +517,14 @@ def register_node_kind( contains ".", so a namespaced kind can never literally collide with one, and no separate reserved-kind blocklist is needed. - v1 ONLY supports requires_parent=True. False is rejected outright: - PluginPicker.tsx's executePlugin(name, parentId) wire call carries - no x/y at all, so a parentless node has no host-decided place to - spawn. + `requires_parent=False` registers a kind that can be created on an + empty canvas with nothing selected. It was rejected outright in v1 + because executePlugin's wire call carried no x/y, leaving a + parentless node with no host-decided place to spawn; the call now + carries the viewport center, surfaced to the factory as + PluginRunContext.spawn_x/spawn_y, so the position exists and the + restriction is gone. The factory is invoked with parent_node_id=None + in that case, and the created node is left unconnected. ADR-014 stage 14.2: 'serialize'/'deserialize' are the OPTIONAL generic persistence/wire seam for a plugin's own NodeState subclass @@ -535,12 +555,6 @@ def register_node_kind( here is caught by the caller - the node still restores with its title/content, just without its extra state - never a failed load.""" - if not requires_parent: - raise PluginRegistrationError( - f'plugin "{self.plugin_id}": requires_parent=False is not supported in SDK ' - f"v1 - the picker's executePlugin(name, parentId) call carries no spawn " - f'position for a parentless node.' - ) if not kind or not all(c.isalnum() or c == "_" for c in kind): raise PluginRegistrationError( f'plugin "{self.plugin_id}": invalid node kind "{kind}" ' @@ -552,7 +566,8 @@ def register_node_kind( f'plugin "{self.plugin_id}": node kind "{kind}" already registered' ) self._node_kinds[namespaced] = NodeKindSpec( - plugin_id=self.plugin_id, kind=namespaced, factory=factory, requires_parent=True, + plugin_id=self.plugin_id, kind=namespaced, factory=factory, + requires_parent=requires_parent, serialize=serialize, deserialize=deserialize, ) @@ -585,6 +600,7 @@ def register_picker_entry( def register_builtin_plugin( self, *, name: str, description: str, category: str, handler: BuiltinActionHandler, + requires_parent: bool = True, ) -> None: """ADR-014 stage 14.3: the first-party migration escape hatch - lets a plugin's register() call attach a picker entry directly to an @@ -646,7 +662,7 @@ async def would be needless ceremony this call path has no use for. ) self._builtin_actions[name] = BuiltinActionSpec( plugin_id=self.plugin_id, name=name, description=description, - category=category, handler=handler, + category=category, handler=handler, requires_parent=requires_parent, ) def register_intent( diff --git a/backend/plugins.py b/backend/plugins.py index 8ce4e5c1..2e0d819d 100644 --- a/backend/plugins.py +++ b/backend/plugins.py @@ -71,6 +71,12 @@ class ExecutePluginArgs: plugin_name: str parent_node_id: str | None = None + # The canvas viewport's center at click time, in scene coordinates. + # Carried for every call, but only READ when the resolved plugin does + # not require a parent - it is where such a node is spawned. Optional so + # an older caller (or a test) that omits it still dispatches. + spawn_x: float | None = None + spawn_y: float | None = None @dataclass @@ -279,6 +285,8 @@ async def _execute_discovered_plugin( settings_manager: SettingsManager, name: str, parent_node_id: str | None, + spawn_x: float | None = None, + spawn_y: float | None = None, ): """The single executePlugin dispatch path for EVERY name - built-in or third-party alike, since ADR-014 stage 14.3 migrated the last hardcoded @@ -317,7 +325,10 @@ async def _execute_discovered_plugin( regardless of which mechanism a real match would have used.""" builtin_spec = plugin_registry.resolve_builtin_action(name) if builtin_spec is not None: - run_ctx = PluginRunContext(plugin_id=builtin_spec.plugin_id, notifications=notifications) + run_ctx = PluginRunContext( + plugin_id=builtin_spec.plugin_id, notifications=notifications, + spawn_x=spawn_x, spawn_y=spawn_y, + ) result = builtin_spec.handler(canvas_document, run_ctx, parent_node_id) await bus.publish("scene" if result is not None else "notification") return result @@ -337,25 +348,38 @@ async def _execute_discovered_plugin( await bus.publish("notification") return None - if not parent_node_id or parent_node_id not in canvas_document.nodes: + # A kind registered with requires_parent=False is creatable with nothing + # selected - it spawns at the viewport center the picker reported rather + # than at a place_child offset from a parent. Only a kind that genuinely + # needs a branch to attach to still refuses without one. + has_parent = bool(parent_node_id) and parent_node_id in canvas_document.nodes + if kind_spec.requires_parent and not has_parent: notifications.show( f'Please select a valid node to branch from before adding a ' f'{picker_entry.name} node.', "warning", ) await bus.publish("notification") return None - run_ctx = PluginRunContext(plugin_id=kind_spec.plugin_id, notifications=notifications) + effective_parent = parent_node_id if has_parent else None + run_ctx = PluginRunContext( + plugin_id=kind_spec.plugin_id, notifications=notifications, + spawn_x=spawn_x, spawn_y=spawn_y, + ) def _mutator(): - seed = kind_spec.factory(canvas_document, run_ctx, parent_node_id) - x, y = canvas_document.place_child(parent_node_id, kind_spec.kind) + seed = kind_spec.factory(canvas_document, run_ctx, effective_parent) + if effective_parent is not None: + x, y = canvas_document.place_child(effective_parent, kind_spec.kind) + else: + x, y = float(spawn_x or 0.0), float(spawn_y or 0.0) return canvas_document.add_plugin_node( - kind_spec.kind, x, y, parent_node_id, + kind_spec.kind, x, y, effective_parent, title=seed.title, content=seed.content, state=seed.state, ) node, _command = canvas_document.record_command( - f"plugin:{kind_spec.kind}", "user", _mutator, node_ids=[parent_node_id], + f"plugin:{kind_spec.kind}", "user", _mutator, + node_ids=[effective_parent] if effective_parent is not None else [], ) await bus.publish("scene") return node.id @@ -694,7 +718,10 @@ def register_plugins( # _plugin_grants_payload's own docstring. bus.register_topic("app-plugins", lambda: plugins_payload(plugin_registry, settings_manager)) - async def execute_plugin(plugin_name: str, parent_node_id: str | None = None): + async def execute_plugin( + plugin_name: str, parent_node_id: str | None = None, + spawn_x: float | None = None, spawn_y: float | None = None, + ): name = str(plugin_name).strip() # ADR-014 stage 14.3: every name - built-in or third-party alike - # flows through the same generic dispatch helper now. See @@ -702,7 +729,8 @@ async def execute_plugin(plugin_name: str, parent_node_id: str | None = None): # registration mechanisms it checks, in order, and (stage 14.4) the # grant gate the generic path now enforces. return await _execute_discovered_plugin( - bus, notifications, canvas_document, plugin_registry, settings_manager, name, parent_node_id, + bus, notifications, canvas_document, plugin_registry, settings_manager, name, + parent_node_id, spawn_x, spawn_y, ) bus.register_intent("app-plugins", "executePlugin", execute_plugin, args_schema=ExecutePluginArgs) diff --git a/plugins/counter_node/plugin.py b/backend/tests/fixture_plugins/counter_node/plugin.py similarity index 100% rename from plugins/counter_node/plugin.py rename to backend/tests/fixture_plugins/counter_node/plugin.py diff --git a/plugins/counter_node/plugin.toml b/backend/tests/fixture_plugins/counter_node/plugin.toml similarity index 100% rename from plugins/counter_node/plugin.toml rename to backend/tests/fixture_plugins/counter_node/plugin.toml diff --git a/plugins/hello_node/plugin.py b/backend/tests/fixture_plugins/hello_node/plugin.py similarity index 100% rename from plugins/hello_node/plugin.py rename to backend/tests/fixture_plugins/hello_node/plugin.py diff --git a/plugins/hello_node/plugin.toml b/backend/tests/fixture_plugins/hello_node/plugin.toml similarity index 100% rename from plugins/hello_node/plugin.toml rename to backend/tests/fixture_plugins/hello_node/plugin.toml diff --git a/plugins/sandboxed_demo/plugin.py b/backend/tests/fixture_plugins/sandboxed_demo/plugin.py similarity index 100% rename from plugins/sandboxed_demo/plugin.py rename to backend/tests/fixture_plugins/sandboxed_demo/plugin.py diff --git a/plugins/sandboxed_demo/plugin.toml b/backend/tests/fixture_plugins/sandboxed_demo/plugin.toml similarity index 100% rename from plugins/sandboxed_demo/plugin.toml rename to backend/tests/fixture_plugins/sandboxed_demo/plugin.toml diff --git a/backend/tests/test_app_ws.py b/backend/tests/test_app_ws.py index fce01bfe..cb9f3975 100644 --- a/backend/tests/test_app_ws.py +++ b/backend/tests/test_app_ws.py @@ -310,13 +310,15 @@ def test_executeplugin_rejects_wrong_arity_before_running_the_handler(): "kind": "intent", "topic": "app-plugins", "intent": "executePlugin", - "args": ["System Prompt", "node-1", "unexpected-extra"], + # executePlugin takes (name, parent_node_id, spawn_x, + # spawn_y); a fifth argument is the arity error. + "args": ["System Prompt", "node-1", 0, 0, "unexpected-extra"], "id": 8, } ) message = ws.receive_json() assert message["kind"] == "error" - assert message["error"] == "Invalid arguments: expected at most 2 argument(s), got 3." + assert message["error"] == "Invalid arguments: expected at most 4 argument(s), got 5." def test_junk_args_never_crash_or_disconnect_any_registered_intent(): diff --git a/backend/tests/test_plugin_sdk.py b/backend/tests/test_plugin_sdk.py index 9dc182fa..3e25821c 100644 --- a/backend/tests/test_plugin_sdk.py +++ b/backend/tests/test_plugin_sdk.py @@ -12,6 +12,7 @@ import asyncio import logging import textwrap +from pathlib import Path import pytest @@ -29,6 +30,15 @@ from backend.session_save import build_chat_data from graphlink_settings_store import SettingsManager +# The three demo plugins (hello_node, counter_node, sandboxed_demo) live in +# backend/tests/fixture_plugins/, not in the shipped plugins/ root: they are +# test fixtures, and while they sat in plugins/ they were packaged into the +# wheel by MANIFEST.in and listed in every user's plugin picker as "Hello +# Node" / "Counter Node" / "Sandboxed Env Probe". They are still REAL plugins +# loaded through real discovery - that is the point of them - just discovered +# from a root that ships to nobody. +FIXTURE_PLUGINS_ROOT = Path(__file__).resolve().parent / "fixture_plugins" + def _write_plugin( plugins_root, @@ -216,18 +226,35 @@ def test_picker_name_collision_between_two_plugins_is_recorded_first_wins_not_ov assert "plugin_b.greet" not in registry.node_kinds -# -- requires_parent=False: rejected at REGISTRATION time ------------------- +# -- requires_parent: both values are real registrations now ---------------- +# +# requires_parent=False used to raise here, because executePlugin's wire call +# carried no x/y and a parentless node had nowhere host-decided to spawn. The +# call now reports the canvas viewport's center, so the position exists and +# the kind registers - which is what lets a plugin be created on an empty +# canvas instead of demanding a pre-existing selected node. -def test_requires_parent_false_raises_plugin_registration_error_at_registration_time(): +def test_requires_parent_false_registers_and_is_reported_on_the_spec(): host = HostContext("some_plugin") def _factory(document, run_ctx, parent_id): - raise AssertionError("factory must never be called - registration itself must fail") + raise AssertionError("factory is not called at registration time") - with pytest.raises(PluginRegistrationError): - host.register_node_kind("floating", _factory, requires_parent=False) - assert "some_plugin.floating" not in host._node_kinds + host.register_node_kind("floating", _factory, requires_parent=False) + + assert host._node_kinds["some_plugin.floating"].requires_parent is False + + +def test_requires_parent_defaults_to_true(): + host = HostContext("some_plugin") + + def _factory(document, run_ctx, parent_id): + raise AssertionError("factory is not called at registration time") + + host.register_node_kind("child", _factory) + + assert host._node_kinds["some_plugin.child"].requires_parent is True # -- HostContext.register_intent(): declaration-time behavior only ---------- @@ -531,8 +558,8 @@ def test_undo_reverts_a_plugin_created_node_and_its_parent_edge(tmp_path): # # Every test below deliberately uses a FRESH tmp_path plugin, never known to # session_save.py/session_load.py's own source code, to prove the mechanism -# is genuinely generic - not a special case for plugins/hello_node/ or -# plugins/counter_node/ (which get their own, separate, real-plugin- +# is genuinely generic - not a special case for the hello_node or +# counter_node fixtures (which get their own, separate, real-plugin- # integration coverage further down). No test in this section adds a single # line to session_save.py/session_load.py to pass - that IS the proof that # a future plugin needs zero edits there. @@ -836,13 +863,14 @@ def register(host: HostContext) -> None: # # The tests above prove the MECHANISM is generic against an arbitrary, # never-before-seen plugin. These two prove the actual shipped -# plugins/hello_node/ and plugins/counter_node/ (real discover_plugins(), -# no tmp_path override) round-trip correctly through the real default -# discovery root - the production configuration, not just the test harness. +# hello_node/ and counter_node/ (real discover_plugins() against the +# fixture root) round-trip correctly through real discovery - the same code +# path production uses, exercised by real plugin packages rather than by a +# hand-built tmp_path stub. def test_real_hello_node_plugin_round_trips_through_save_and_reload(tmp_path): - registry = discover_plugins() + registry = discover_plugins(FIXTURE_PLUGINS_ROOT) bus, notifications, canvas_document = _make_wired_bus(registry, tmp_path) parent = canvas_document.add_chat_node(10, 20, "parent", is_user=False) @@ -860,7 +888,7 @@ def test_real_hello_node_plugin_round_trips_through_save_and_reload(tmp_path): def test_real_counter_node_plugin_round_trips_its_custom_state_through_save_and_reload(tmp_path): - registry = discover_plugins() + registry = discover_plugins(FIXTURE_PLUGINS_ROOT) bus, notifications, canvas_document = _make_wired_bus(registry, tmp_path) parent = canvas_document.add_chat_node(10, 20, "parent", is_user=False) diff --git a/backend/tests/test_plugin_worker.py b/backend/tests/test_plugin_worker.py index ab022938..2df29cd3 100644 --- a/backend/tests/test_plugin_worker.py +++ b/backend/tests/test_plugin_worker.py @@ -13,7 +13,7 @@ picker entry appears in plugins_payload() exactly like an in-process plugin's would. C. THE empirical secret-containment proof: a fake secret-shaped env var is - planted on the HOST test process, the shipped plugins/sandboxed_demo + planted on the HOST test process, the sandboxed_demo fixture plugin plugin's node-creation is triggered, and the created node's content - built entirely inside the WORKER subprocess - is asserted to contain NEITHER the planted value NOR the env var's own name. @@ -47,6 +47,7 @@ import shutil import textwrap import time +from pathlib import Path import pytest @@ -54,7 +55,6 @@ from backend.events import SessionBus from backend.notifications import NotificationState from backend.plugin_sdk import ( - DEFAULT_PLUGINS_ROOT, PluginRegistrationError, PluginWorkerClient, PluginWorkerError, @@ -67,6 +67,34 @@ from backend.tools import RunContext, ToolRegistry from graphlink_settings_store import SettingsManager +# The three demo plugins (hello_node, counter_node, sandboxed_demo) live in +# backend/tests/fixture_plugins/, not in the shipped plugins/ root: they are +# test fixtures, and while they sat in plugins/ they were packaged into the +# wheel by MANIFEST.in and listed in every user's plugin picker as "Hello +# Node" / "Counter Node" / "Sandboxed Env Probe". They are still REAL plugins +# loaded through real discovery - that is the point of them - just discovered +# from a root that ships to nobody. +FIXTURE_PLUGINS_ROOT = Path(__file__).resolve().parent / "fixture_plugins" + + +@pytest.fixture +def sandboxed_demo_is_discoverable(monkeypatch): + """Points backend/agent_dispatch/builder.py's own discover_plugins() at + the fixture root. + + These tests assert the REAL tool-registry wiring for an out-of-process + plugin, end to end. sandboxed_demo used to sit in the shipped plugins/ + root, so the default discovery found it for free - at the cost of every + user seeing "Sandboxed Env Probe" in their picker. The plugin is a + fixture now, so the test has to say where it lives; the wiring under + test is unchanged.""" + from backend.plugin_sdk import discover_plugins as _discover + + monkeypatch.setattr( + "backend.plugin_sdk.discover_plugins", + lambda *args, **kwargs: _discover(FIXTURE_PLUGINS_ROOT), + ) + def _close_workers(registry) -> None: for client in registry.worker_clients.values(): @@ -292,7 +320,7 @@ def test_out_of_process_plugin_worker_cannot_see_a_planted_host_secret_env_var(t """THE stage 14.5 exit criterion, proven empirically: plant GRAPHLINK_OPENAI_API_KEY=fake-test-secret-value on the HOST test process, discover a fresh copy of the REAL shipped - plugins/sandboxed_demo plugin (a fresh tmp_path copy, so the env var is + sandboxed_demo fixture plugin (a fresh tmp_path copy, so the env var is guaranteed to be set BEFORE this specific worker's Popen call, and this test never touches the shared, memoized real-repo-root registry any other test might have already populated), grant it, create its node, @@ -303,7 +331,7 @@ def test_out_of_process_plugin_worker_cannot_see_a_planted_host_secret_env_var(t plugins_root = tmp_path / "plugins" plugins_root.mkdir() - shutil.copytree(DEFAULT_PLUGINS_ROOT / "sandboxed_demo", plugins_root / "sandboxed_demo") + shutil.copytree(FIXTURE_PLUGINS_ROOT / "sandboxed_demo", plugins_root / "sandboxed_demo") registry = discover_plugins(plugins_root) try: @@ -860,7 +888,7 @@ async def request_approval(call): # -- G: backend/agents.py's builder_tool_registry() really wires it in ------ -def test_builder_tool_registry_includes_the_real_shipped_sandboxed_demo_plugin_tool(tmp_path): +def test_builder_tool_registry_includes_the_real_out_of_process_plugin_tool(tmp_path, sandboxed_demo_is_discoverable): from backend.agents import AgentDispatcher settings_manager = SettingsManager(tmp_path / "session.dat") @@ -1049,7 +1077,7 @@ def test_out_of_process_plugin_node_state_round_trips_through_save_and_reload(tm # -- ADR-021 stage 21.4: plugin intent arguments, end to end ---------------- -def test_the_real_sandboxed_plugin_tool_advertises_its_declared_arguments(tmp_path): +def test_the_real_sandboxed_plugin_tool_advertises_its_declared_arguments(tmp_path, sandboxed_demo_is_discoverable): """The host never imports plugins/sandboxed_demo/plugin.py, so it cannot see PingArgs at all - the worker generates the schema on its own side and sends it up at registration time. Before stage 21.4 this spec's @@ -1072,7 +1100,7 @@ def test_the_real_sandboxed_plugin_tool_advertises_its_declared_arguments(tmp_pa assert spec.input_schema["required"] == ["message"] -def test_arguments_reach_an_out_of_process_intent_and_come_back(tmp_path): +def test_arguments_reach_an_out_of_process_intent_and_come_back(tmp_path, sandboxed_demo_is_discoverable): """The full stage-21.4 round trip across the isolation boundary: the host forwards a raw dict (it has no access to the plugin's dataclass), the worker validates and constructs PingArgs against the real type, and the @@ -1100,7 +1128,7 @@ def test_arguments_reach_an_out_of_process_intent_and_come_back(tmp_path): assert result.content == "pong from sandboxed_demo: hi hi hi" -def test_invalid_arguments_come_back_as_tool_feedback_not_a_crash(tmp_path): +def test_invalid_arguments_come_back_as_tool_feedback_not_a_crash(tmp_path, sandboxed_demo_is_discoverable): """A wrong-typed argument is ordinary tool feedback the loop hands back to the model to correct - the posture ToolRegistry.invoke() takes for every expected denial - not an exception out of the turn.""" diff --git a/backend/tests/test_plugins.py b/backend/tests/test_plugins.py index 7c3a68a8..cc309237 100644 --- a/backend/tests/test_plugins.py +++ b/backend/tests/test_plugins.py @@ -15,6 +15,10 @@ from backend.plugins import get_plugin_categories, plugins_payload, register_plugins from graphlink_settings_store import SettingsManager +# The demo plugins are fixtures, not shipped plugins - see +# backend/tests/fixture_plugins/ and this module's header comment. +FIXTURE_PLUGINS_ROOT = Path(__file__).resolve().parent / "fixture_plugins" + def _fresh_settings_manager() -> SettingsManager: # ADR-014 stage 14.4: register_plugins now requires a real SettingsManager @@ -33,11 +37,15 @@ def _fresh_settings_manager() -> SettingsManager: # supply entries even with no real registry. Since the 8 built-ins are now # real discovered plugins themselves, there is no picker entry that exists # independent of a real registry anymore - these tests now pass the real -# discovered registry (discover_plugins(), the production plugins/ root), -# which also picks up plugins/hello_node/ and plugins/counter_node/ (both -# registered under the HostContext default "More Plugins" category) - a +# discovered registry (discover_plugins(), the production plugins/ root) - a # genuine, expected structural difference from the pre-migration synthetic # "just the hardcoded 8" listing, not a narrowing of intent. +# +# The shipped root holds exactly the seven first-party plugins. The demo +# plugins that used to sit alongside them - and so appeared in every user's +# picker under "More Plugins" - are fixtures now, discovered from +# backend/tests/fixture_plugins/, so the catch-all category's behavior is +# proven against that root instead. def test_get_plugin_categories_groups_in_category_order_and_skips_empty(): @@ -46,12 +54,11 @@ def test_get_plugin_categories_groups_in_category_order_and_skips_empty(): # "Validation & Delivery" has no plugins mapped to it in the real # shipped plugin set today, so the empty-category-skip rule drops it - # from the result entirely. "More Plugins" is appended last for - # plugins/hello_node/'s and plugins/counter_node/'s uncategorized - # entries. + # from the result entirely. Every shipped plugin declares a real + # category, so the "More Plugins" catch-all is correctly absent. assert names == [ "Branch Foundations", "Reasoning & Research", "Build & Execution", - "Workflow & Drafting", "More Plugins", + "Workflow & Drafting", ] for category in grouped: assert category["plugins"] @@ -82,11 +89,11 @@ def test_get_plugin_categories_pins_the_original_curated_within_category_order_f def test_get_plugin_categories_appends_more_plugins_only_if_uncategorized(): - # Real discovery includes plugins/hello_node/ and plugins/counter_node/, - # both registered with the HostContext default category ("More - # Plugins") - proves the catch-all activates for genuinely - # uncategorized entries. - grouped = get_plugin_categories(discover_plugins()) + # The fixture root's demo plugins register with the HostContext default + # category ("More Plugins"), which is what proves the catch-all + # activates for genuinely uncategorized entries. No SHIPPED plugin is + # uncategorized any more - see this module's own header comment. + grouped = get_plugin_categories(discover_plugins(FIXTURE_PLUGINS_ROOT)) more_plugins = next(c for c in grouped if c["name"] == "More Plugins") plugin_names = {p["name"] for p in more_plugins["plugins"]} assert {"Hello Node", "Counter Node"} <= plugin_names @@ -97,6 +104,21 @@ def test_get_plugin_categories_appends_more_plugins_only_if_uncategorized(): assert "More Plugins" not in [category["name"] for category in grouped_empty] +def test_no_demo_plugin_ships_in_the_picker(): + """The shipped root is exactly the seven first-party plugins. + + hello_node/counter_node/sandboxed_demo were packaged into the wheel by + MANIFEST.in's `recursive-include plugins` and listed in every user's + picker as "Hello Node" / "Counter Node" / "Sandboxed Env Probe".""" + names = { + plugin["name"] + for category in get_plugin_categories(discover_plugins()) + for plugin in category["plugins"] + } + + assert not names & {"Hello Node", "Counter Node", "Sandboxed Env Probe"} + + def test_get_plugin_categories_covers_every_plugin_exactly_once(): registry = discover_plugins() grouped = get_plugin_categories(registry) @@ -466,34 +488,76 @@ def topics_seen(self): # send time. -def test_execute_plugin_system_prompt_requires_parent(): +def test_execute_plugin_system_prompt_with_no_selection_creates_a_pending_note(): + """A system prompt is authored BEFORE the branch it governs. + + Requiring a selected node meant sending a message - spending tokens - + purely to unlock the action that should have shaped that message. With + nothing selected the note is created unattached, at the spawn point the + picker reports.""" bus, notifications, canvas_document = _make_plugins_bus() - result = asyncio.run(bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt"])) + result = asyncio.run( + bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt", None, 120.0, -40.0]) + ) - assert result is None - assert notifications.visible is True - assert notifications.msg_type == "warning" - assert notifications.message == ( - "Please select a valid node to branch from before adding a System Prompt node." + assert result is not None + note = canvas_document.nodes[result] + assert note.kind == "note" + assert note.state.is_system_prompt is True + assert (note.x, note.y) == (120.0, -40.0) + # Unattached by design - a new branch root adopts it later. + assert not any(edge.source == note.id for edge in canvas_document.edges.values()) + assert notifications.visible is False + + +def test_execute_plugin_system_prompt_with_no_selection_reuses_the_pending_note(): + """Same dedup posture as the attached path: a second pending note would + be just as inert as a second attached one.""" + bus, _notifications, canvas_document = _make_plugins_bus() + + first = asyncio.run( + bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt", None, 0.0, 0.0]) + ) + second = asyncio.run( + bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt", None, 500.0, 500.0]) ) - assert not any(n.kind == "note" for n in canvas_document.nodes.values()) + + assert first == second + assert sum(1 for n in canvas_document.nodes.values() if n.kind == "note") == 1 -def test_execute_plugin_system_prompt_rejects_unknown_parent_id(): +def test_execute_plugin_system_prompt_treats_an_unknown_parent_id_as_no_selection(): + """A stale or deleted selection is simply "nothing is selected", which is + now a supported path rather than a blocking warning.""" bus, notifications, canvas_document = _make_plugins_bus() result = asyncio.run( - bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt", "ghost-node-id"]) + bus.dispatch_intent( + "app-plugins", "executePlugin", ["System Prompt", "ghost-node-id", 10.0, 20.0] + ) ) - assert result is None - assert notifications.visible is True - assert notifications.msg_type == "warning" - assert notifications.message == ( - "Please select a valid node to branch from before adding a System Prompt node." + assert result is not None + assert canvas_document.nodes[result].state.is_system_prompt is True + assert notifications.visible is False + + +def test_a_new_branch_root_adopts_the_pending_system_prompt_note(): + """The note -> root edge _resolve_branch_system_prompt looks for is drawn + when the first parentless chat node appears, so a prompt written first + actually governs the conversation it was written for.""" + bus, _notifications, canvas_document = _make_plugins_bus() + note_id = asyncio.run( + bus.dispatch_intent("app-plugins", "executePlugin", ["System Prompt", None, 0.0, 0.0]) + ) + + root = canvas_document.add_chat_node(0.0, 200.0, "first message", True) + + assert any( + edge.source == note_id and edge.target == root.id + for edge in canvas_document.edges.values() ) - assert not any(n.kind == "note" for n in canvas_document.nodes.values()) def test_execute_plugin_system_prompt_creates_a_note_attached_to_the_branch_root_not_the_selected_child(): diff --git a/plugins/system_prompt/plugin.py b/plugins/system_prompt/plugin.py index 7d7c54f6..58a682a4 100644 --- a/plugins/system_prompt/plugin.py +++ b/plugins/system_prompt/plugin.py @@ -4,6 +4,11 @@ shape genuinely differs from the other 7 (a note placed ABOVE the branch root, not a child fanned below the selected node): +- With NO selection, creates the note unattached at the picker's reported + viewport center; SceneDocument.adopt_pending_system_prompt then connects + it to the first branch root created afterwards. A system prompt is + authored before the branch it governs, so requiring an existing node + (the pre-fix behavior) forced a wasted send just to unlock the action. - Resolves parent_node_id's BRANCH ROOT (SceneDocument.get_branch_root), the same parent-edge walk backend/agents.py's _resolve_branch_system_prompt uses at send time - the note attaches to @@ -32,11 +37,14 @@ def _execute( document: SceneDocument, run_ctx: PluginRunContext, parent_node_id: "str | None", ) -> "str | None": if not parent_node_id or parent_node_id not in document.nodes: - run_ctx.notifications.show( - "Please select a valid node to branch from before adding a System Prompt node.", - "warning", - ) - return None + # No selection: create the prompt UNATTACHED, at the viewport center. + # A system prompt is the one node that is naturally authored BEFORE + # the branch it governs - requiring a node to exist first meant + # sending a message just to earn the right to set the prompt that + # should have shaped it. SceneDocument.adopt_pending_system_prompt + # connects this note to the first branch root that appears, which is + # the note -> root edge _resolve_branch_system_prompt looks for. + return _create_pending(document, run_ctx) root = document.get_branch_root(parent_node_id) existing = next( ( @@ -65,6 +73,37 @@ def _create_system_prompt_note(): return note.id +def _create_pending( + document: SceneDocument, run_ctx: PluginRunContext, +) -> "str | None": + """Creates (or reuses) the unattached system-prompt note. + + Reuse mirrors the attached path's own dedup rule: a second pending note + would be just as inert as a second attached one, since only the first + can ever be adopted by a root.""" + existing = next( + ( + node + for node in document.nodes.values() + if node.kind == "note" + and node.state.is_system_prompt + and not any(edge.source == node.id for edge in document.edges.values()) + ), + None, + ) + if existing is not None: + return existing.id + + x = float(run_ctx.spawn_x or 0.0) + y = float(run_ctx.spawn_y or 0.0) + + def _create(): + return document.add_note(x, y, is_system_prompt=True) + + note, _command = document.record_command("pluginSystemPrompt", "user", _create) + return note.id + + def register(host: HostContext) -> None: host.register_builtin_plugin( name="System Prompt", @@ -74,4 +113,6 @@ def register(host: HostContext) -> None: ), category="Branch Foundations", handler=_execute, + # Creatable on an empty canvas: see _execute's own no-selection path. + requires_parent=False, ) diff --git a/web_ui/e2e/create-node.spec.ts b/web_ui/e2e/create-node.spec.ts index 77a19cb7..d2fcf34b 100644 --- a/web_ui/e2e/create-node.spec.ts +++ b/web_ui/e2e/create-node.spec.ts @@ -5,53 +5,50 @@ import { gotoApp } from "./helpers"; * ADR-015 stage 15.6: creates two real nodes through two real UI entry * points, neither needing an LLM call or any network access: * - * 1. Double-click on empty canvas - SceneCanvas.tsx's own "R1 create-node - * gesture" (its onDoubleClick handler), which fires the `addNode` scene - * intent (backend/api/intents_nodes.py) straight into - * SceneDocument.add_node - a pure in-memory mutation, no provider call - * anywhere in that path. This is also the only way to get a FIRST node - * onto an empty canvas: every plugin-picker entry (below) requires an - * already-selected parent node, so there is no other real entry point - * for node #1. - * 2. The Plugins popover (AppBar.tsx's `[data-overlay-trigger="plugins"]`) - * -> "Conversation Node" - one of the 7 first-party plugins migrated - * onto ADR-014's `register_builtin_plugin` escape hatch - * (plugins/conversation_node/plugin.py). Picked over every other - * picker entry specifically because its handler + * 1. The Plugins popover with NOTHING selected -> "System Prompt". This is + * the empty-canvas entry point. It did not exist until plugins could be + * created without a parent: every picker entry required an + * already-selected node, so the only way to get node #1 onto an empty + * canvas was a double-click gesture that produced an untitled + * placeholder, and this spec used to bootstrap through it. System Prompt + * is registered requires_parent=False and its handler creates the note + * at the picker's reported spawn point (plugins/system_prompt/plugin.py). + * 2. The Plugins popover again, WITH that node selected -> "Conversation + * Node" - one of the 7 first-party plugins migrated onto ADR-014's + * `register_builtin_plugin` escape hatch. Picked over every other picker + * entry specifically because its handler * (SceneDocument.add_conversation_node) does nothing but create and * connect a node - unlike, say, Web Research or the Virtual Environment - * Runner, it never calls out to a provider or a subprocess, so this - * stays exactly as deterministic and offline as the double-click gesture above. - * Built-in plugins also bypass the install-time consent/grant gate - * entirely (see backend/plugins.py's own module docstring on - * `builtin_actions` vs `picker_entries`), so there is no extra Settings - * > Plugins step needed before this can run. + * Runner, it never calls out to a provider or a subprocess, so this stays + * fully deterministic and offline. Built-in plugins also bypass the + * install-time consent/grant gate entirely (see backend/plugins.py's own + * module docstring on `builtin_actions` vs `picker_entries`), so no + * Settings > Plugins step is needed first. + * + * Together they pin both halves of the creation contract: a plugin that + * needs no parent works on a blank canvas, and one that does still branches + * off the selection. */ -test("creates a node by double-click, then a Conversation Node via the Plugins picker", async ({ page }) => { +test("creates a node from an empty canvas, then a Conversation Node via the Plugins picker", async ({ page }) => { await gotoApp(page); - const canvas = page.getByTestId("scene-canvas"); - const box = await canvas.boundingBox(); - if (!box) throw new Error("scene-canvas has no layout box to click into"); - // A corner offset rather than the canvas's exact center. This used to be - // load-bearing: a full-canvas empty-state hint was centered over the - // canvas until the first node existed, so a dblclick at width/2,height/2 - // landed on its "Load Sample Workspace" button instead of blank canvas. - // That overlay is gone, so any point on the canvas would do now - the - // offset is kept because the exact coordinates are arbitrary either way - // and moving them would churn a passing test for nothing. - await canvas.dblclick({ position: { x: box.width * 0.15, y: box.height * 0.15 } }); + await expect(page.locator(".react-flow__node")).toHaveCount(0); + + // Nothing selected: the picker itself is the creation path now. + await page.locator('[data-overlay-trigger="plugins"]').click(); + // "Branch Foundations" (backend/plugins.py's _CATEGORY_META) is the + // picker's own default active category, and both entries below belong to + // it, so no category click is needed. + await page.getByRole("option", { name: "System Prompt" }).click(); await expect(page.locator(".react-flow__node")).toHaveCount(1); + await expect(page.locator(".react-flow__node-note")).toHaveCount(1); - // Select the freshly created node - Conversation Node's own handler - // requires a valid parent_node_id (plugins/conversation_node/plugin.py). + // Select it - Conversation Node's own handler DOES require a valid + // parent_node_id (plugins/conversation_node/plugin.py). await page.locator(".react-flow__node").first().click(); await page.locator('[data-overlay-trigger="plugins"]').click(); - // "Branch Foundations" (backend/plugins.py's _CATEGORY_META) is the - // picker's own default active category, so no extra category click is - // needed - Conversation Node is a member of it. await page.getByRole("option", { name: "Conversation Node" }).click(); await expect(page.locator(".react-flow__node")).toHaveCount(2); diff --git a/web_ui/e2e/workspace-save-reload.spec.ts b/web_ui/e2e/workspace-save-reload.spec.ts index b2e1b241..64cdbcc0 100644 --- a/web_ui/e2e/workspace-save-reload.spec.ts +++ b/web_ui/e2e/workspace-save-reload.spec.ts @@ -27,52 +27,42 @@ import { gotoApp } from "./helpers"; * * The node saved/reloaded here is a System Prompt note (the "System Prompt" * plugin action, plugins/system_prompt/plugin.py), NOT a Conversation Node - - * deliberately, and NOT the bare double-click placeholder alone either. - * Empirically confirmed while building this suite (backend/session_load.py's - * `_restore_node`, read directly): + * deliberately. Empirically confirmed while building this suite + * (backend/session_load.py's `_restore_node`, read directly): * - * - A double-click's own default `kind="placeholder"` (backend/domain/ - * model.py) is excluded from `_REGULAR_KINDS` entirely - session_save.py's - * `_build_chat_data` never serializes it, so a canvas holding ONLY a - * placeholder saves a real chats.db row whose own node list is empty. - * - "conversation" (and html/pycoder/code_sandbox/web_research/artifact/ - * gitlink) is in `_PARENT_NODE_INDEX_KINDS`, which `_restore_node` - * REQUIRES a resolvable parent index for - `if parent_new_id is None: - * return None, None`. A Conversation Node parented off a placeholder saves - * fine (its own `parent_node_index` is simply `null`) but is then SILENTLY - * DROPPED on load, because null is never a resolvable index. There is no - * real, LLM-free UI path to a first, persistable "conversation"-kind node - * on a genuinely empty canvas: bootstrapping one always means parenting it - * off the one parentless creation gesture there is (double-click), and - * that gesture's own node is exactly the unpersisted kind this drops. - * - A Note (`kind="note"`, System Prompt's own handler) has NEITHER problem: - * notes are NOT part of `_REGULAR_KINDS`/the node-index/parent-resolution - * machinery above at all - they serialize into their own `notes_data` list - * (position-only, by backend/session_save.py's own notes-are-separate - * design) and restore via `_restore_notes`, which never resolves a parent - * index. The placeholder here is still needed as System Prompt's own - * required `parent_node_id` argument (branch-root lookup), but the NOTE it - * creates round-trips through save/load independent of that placeholder - * parent's own unpersisted fate - confirmed empirically before writing - * this spec's final assertions. + * - "conversation" (and html/code_sandbox/web_research/artifact/gitlink) is + * in `_PARENT_NODE_INDEX_KINDS`, which `_restore_node` REQUIRES a + * resolvable parent index for - `if parent_new_id is None: return None, + * None`. A node of those kinds with no resolvable parent saves fine (its + * `parent_node_index` is simply `null`) but is then SILENTLY DROPPED on + * load, so none of them is a valid subject for a save/reload assertion + * made from a single-node canvas. + * - A Note (`kind="note"`, System Prompt's own handler) has no such + * problem: notes are NOT part of `_REGULAR_KINDS`/the node-index/ + * parent-resolution machinery at all - they serialize into their own + * `notes_data` list (position-only, by backend/session_save.py's own + * notes-are-separate design) and restore via `_restore_notes`, which + * never resolves a parent index. + * + * The note is created directly on the empty canvas. This spec used to + * bootstrap through a double-click placeholder purely to satisfy System + * Prompt's then-required `parent_node_id`; that placeholder was itself an + * unpersisted kind (excluded from `_REGULAR_KINDS`), so the test was + * carrying a node that saving deliberately threw away. System Prompt is + * registered requires_parent=False now, so the setup is simply the real + * thing the assertion is about. */ test("saves a node, clears the canvas, then reloads it from the chat library", async ({ page }) => { await gotoApp(page); - const canvas = page.getByTestId("scene-canvas"); - const box = await canvas.boundingBox(); - if (!box) throw new Error("scene-canvas has no layout box to click into"); - // A corner offset rather than the exact center - see create-node.spec.ts's - // own comment. It used to be dodging the empty-state hint's "Load Sample - // Workspace" button; that overlay is gone, and the offset is kept only - // because the coordinates are arbitrary either way. - await canvas.dblclick({ position: { x: box.width * 0.15, y: box.height * 0.15 } }); - await expect(page.locator(".react-flow__node")).toHaveCount(1); + await expect(page.locator(".react-flow__node")).toHaveCount(0); - await page.locator(".react-flow__node").first().click(); await page.locator('[data-overlay-trigger="plugins"]').click(); await page.getByRole("option", { name: "System Prompt" }).click(); await expect(page.locator(".react-flow__node-note")).toHaveCount(1); + // The note is the ONLY node on the canvas - nothing else to persist, so + // the reload assertion below is unambiguous. + await expect(page.locator(".react-flow__node")).toHaveCount(1); await page.getByRole("button", { name: "Save", exact: true }).click(); diff --git a/web_ui/src/app/canvas/SceneCanvas.tsx b/web_ui/src/app/canvas/SceneCanvas.tsx index aed42886..345afcb8 100644 --- a/web_ui/src/app/canvas/SceneCanvas.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.tsx @@ -2623,24 +2623,37 @@ function CanvasInner({ return () => document.removeEventListener("keydown", onKeyDown); }, [selectedConnectionId, store]); - const onDoubleClick = useCallback( - (event: React.MouseEvent) => { - // Double-click on empty canvas creates a node there - the R1 stand-in - // for the plugin picker / context menu creation paths (R2/R8). - const target = event.target as HTMLElement; - if (!target.closest(".react-flow__node")) { - const position = screenToFlowPosition({ x: event.clientX, y: event.clientY }); - store.addNode(position.x, position.y); - } - }, - [screenToFlowPosition, store], - ); + // The spawn point for anything created with nothing selected - the plugin + // picker's own creation path. Registered as a PROVIDER rather than a + // stored coordinate: this component owns the React Flow transform, and + // asking at click time means a pan or zoom since the last render can + // never hand out a stale point. + // + // This replaces a double-click-to-create-a-blank-node gesture that + // described itself as "the R1 stand-in for the plugin picker / context + // menu creation paths". Those paths shipped; the stand-in did not go + // away, and since every plugin required a pre-existing selected node, it + // had quietly become the only way to put a first node on an empty canvas + // - an undiscoverable double-click that produced an untitled placeholder. + // Plugins can now be created with nothing selected, so the scaffold is + // gone and the picker is the creation path. + useEffect(() => { + const wrapper = canvasWrapperRef.current; + if (!wrapper) return; + store.setViewportCenterProvider(() => { + const rect = wrapper.getBoundingClientRect(); + return screenToFlowPosition({ + x: rect.left + rect.width / 2, + y: rect.top + rect.height / 2, + }); + }); + return () => store.setViewportCenterProvider(null); + }, [screenToFlowPosition, store]); return (
=> this.filterStatuses; getExportInProgress = (): boolean => this.exportInProgress; + /** Where a node created with nothing selected should be placed, in scene + * coordinates. SceneCanvas registers a provider (it owns the React Flow + * transform; nothing outside the provider tree can convert screen to + * scene coordinates), and callers ask at ACTION time rather than reading + * a stored value, so a pan or zoom between renders can never hand out a + * stale point. Falls back to the origin before the canvas has mounted. */ + private viewportCenterProvider: (() => { x: number; y: number }) | null = null; + + setViewportCenterProvider(provider: (() => { x: number; y: number }) | null): void { + this.viewportCenterProvider = provider; + } + + getSpawnPoint = (): { x: number; y: number } => + this.viewportCenterProvider?.() ?? { x: 0, y: 0 }; + // R5.1: no-op-if-unchanged, same discipline as every other setter here that // guards a redundant assignment before paying for an emit() fan-out. setSelectedNodeId(id: string | null): void { diff --git a/web_ui/src/app/chrome/PluginPicker.test.tsx b/web_ui/src/app/chrome/PluginPicker.test.tsx index 75c79723..13d37a7b 100644 --- a/web_ui/src/app/chrome/PluginPicker.test.tsx +++ b/web_ui/src/app/chrome/PluginPicker.test.tsx @@ -100,7 +100,10 @@ describe("PluginPicker", () => { await user.click(screen.getByText("open plugins")); await user.click(screen.getByText("System Prompt")); - expect(intents).toContainEqual(["app-plugins", "executePlugin", ["System Prompt", null]]); + // Spawn point rides along on every call: it is where a plugin that does + // not require a parent lands when nothing is selected. 0,0 here because + // no canvas has registered a viewport-center provider in this test. + expect(intents).toContainEqual(["app-plugins", "executePlugin", ["System Prompt", null, 0, 0]]); // Close-on-select: the popover dismisses so the resulting notification // banner is seen unobstructed (matching the legacy popup's behavior). expect(screen.queryByText("Categories")).toBeNull(); @@ -113,7 +116,7 @@ describe("PluginPicker", () => { await user.click(screen.getByText("open plugins")); await user.click(screen.getByText("System Prompt")); - expect(intents).toContainEqual(["app-plugins", "executePlugin", ["System Prompt", "node-42"]]); + expect(intents).toContainEqual(["app-plugins", "executePlugin", ["System Prompt", "node-42", 0, 0]]); }); it("every plugin's click sends the selected node id, not just Web Research's", async () => { @@ -124,6 +127,34 @@ describe("PluginPicker", () => { await user.click(screen.getByRole("button", { name: /Build & Execution/ })); await user.click(screen.getByText("Virtual Environment Runner")); - expect(intents).toContainEqual(["app-plugins", "executePlugin", ["Virtual Environment Runner", "node-7"]]); + expect(intents).toContainEqual(["app-plugins", "executePlugin", ["Virtual Environment Runner", "node-7", 0, 0]]); + }); + + it("sends the canvas viewport's center as the spawn point when a provider is registered", async () => { + const store = new SceneStore({ subscribe: vi.fn(), intent: vi.fn(), fireIntent: vi.fn() } as unknown as WsTransport); + // SceneCanvas registers this; it owns the React Flow transform, so + // nothing outside the provider tree can convert screen to scene coords. + store.setViewportCenterProvider(() => ({ x: 320, y: -75 })); + const { user, intents } = setup(store); + await user.click(screen.getByText("open plugins")); + + await user.click(screen.getByText("System Prompt")); + expect(intents).toContainEqual([ + "app-plugins", + "executePlugin", + ["System Prompt", null, 320, -75], + ]); + }); + + it("asks the provider at click time, so a pan since the last render is reflected", async () => { + const store = new SceneStore({ subscribe: vi.fn(), intent: vi.fn(), fireIntent: vi.fn() } as unknown as WsTransport); + let center = { x: 0, y: 0 }; + store.setViewportCenterProvider(() => center); + const { user, intents } = setup(store); + await user.click(screen.getByText("open plugins")); + center = { x: 900, y: 900 }; + + await user.click(screen.getByText("System Prompt")); + expect(intents).toContainEqual(["app-plugins", "executePlugin", ["System Prompt", null, 900, 900]]); }); }); diff --git a/web_ui/src/app/chrome/PluginPicker.tsx b/web_ui/src/app/chrome/PluginPicker.tsx index 931b00fc..da0f4c7f 100644 --- a/web_ui/src/app/chrome/PluginPicker.tsx +++ b/web_ui/src/app/chrome/PluginPicker.tsx @@ -12,13 +12,17 @@ import { Popover, useOverlays } from "../overlays/overlays"; * `executePlugin` intent, which surfaces an honest "lands in R3/R5" * notification instead of creating a node - node types don't exist yet. * - * R5.1: every plugin's click now also sends the canvas's currently-selected + * R5.1: every plugin's click also sends the canvas's currently-selected * node id (store.getSelectedNodeId()) as executePlugin's second argument - - * future plugin-node-creation wiring built once, here, rather than - * special-cased per plugin. Most plugins (including Web Research itself) - * don't read this argument yet; it rides along regardless, same posture the - * backend's own execute_plugin(plugin_name, parent_node_id=None) already - * takes. + * plugin-node-creation wiring built once, here, rather than special-cased + * per plugin. + * + * It now sends the canvas viewport's center alongside it. A plugin + * registered with requires_parent=False is creatable with NOTHING selected, + * and this is the position such a node spawns at - the missing piece that + * previously made requires_parent=False unrepresentable and so forced every + * plugin, System Prompt included, to demand a pre-existing node before it + * could run at all. */ const initialState: AppPluginsState = { @@ -90,9 +94,12 @@ export function PluginPicker({ transport, store }: { transport: WsTransport; sto type="button" className="plugin-picker-row" onClick={() => { + const spawn = store.getSpawnPoint(); transport.fireIntent("app-plugins", "executePlugin", [ plugin.name, store.getSelectedNodeId(), + spawn.x, + spawn.y, ]); // Close on select, matching the legacy popup's own // post-execute dismiss - and so the resulting