diff --git a/backend/domain/node_access.py b/backend/domain/node_access.py index 5bbb92c..55de69d 100644 --- a/backend/domain/node_access.py +++ b/backend/domain/node_access.py @@ -90,6 +90,31 @@ def optional_node( return node # type: ignore[return-value] +def with_state(node: SceneNode, state_cls: type[_S]) -> "_NodeWith[_S]": + """A node whose kind has already been decided, narrowed by its state. + + require_node's form for code that is HANDED a node instead of looking one + up. backend/session_save.py's per-kind serializers are the case this + exists for: each is reached through a kind-keyed dispatch table, so the + kind is settled before the function is entered - what the function needs + is not another kind check but a way to say which state it is about to + read, in a form a checker can follow. + + Unlike is_node_of this returns the node rather than a bool, because these + callers have no wrong-kind branch to take: a serializer handed the wrong + node cannot produce a correct payload, and writing a half-built one into + a save file is worse than failing. The isinstance check is redundant on + every live path for the same reason require_node's is - and, exactly like + require_node's, it is the difference between a clear error and a + confusing one if a future dispatch table ever disagrees with itself. + """ + if not isinstance(node.state, state_cls): + raise SceneError( + f"node {node.id} is {node.kind} but has no {state_cls.__name__}" + ) + return node # type: ignore[return-value] + + def is_node_of( node: SceneNode | None, kind: str | tuple[str, ...], state_cls: type[_S], ) -> TypeGuard["_NodeWith[_S]"]: diff --git a/backend/session_load.py b/backend/session_load.py index 5864b5e..678dcc8 100644 --- a/backend/session_load.py +++ b/backend/session_load.py @@ -207,6 +207,10 @@ _content_codec, _placeholder_chart_data, ) +from backend.domain.node_access import with_state +# Not re-exported by backend.canvas, unlike the kinds above - imported +# from the domain package directly, as session_save.py does. +from backend.domain.node_states import ChartState, FrameState, NoteState from backend.plugin_sdk import NodeKindSpec, PluginRegistry, discover_plugins from graphlink_chart_data import ChartDataError, canonicalize_chart_data from graphlink_navigation_pins import NavigationPinRecord @@ -469,7 +473,7 @@ def _restore_chat_payload(payload: dict[str, Any]) -> SceneNode: is_branch_synthesis=bool(payload.get("is_branch_synthesis", False)), synthesis_instructions=str(payload.get("synthesis_instructions", "") or ""), branch_status=( - payload.get("branch_status") + str(payload.get("branch_status")) if payload.get("branch_status") in SceneDocument.BRANCH_STATUS_VALUES else "active" ), @@ -538,7 +542,10 @@ def _restore_image_payload(payload: dict[str, Any], document: SceneDocument) -> import uuid as _uuid x, y = _position(payload) - node = SceneNode(id="", x=x, y=y, title="Image", kind="image", state=ImageState()) + node = with_state( + SceneNode(id="", x=x, y=y, title="Image", kind="image", state=ImageState()), + ImageState, + ) asset_store = _ACTIVE_ASSET_STORE.get() # ADR-009 stage 9.5: READ BOTH SHAPES. A chat saved with an asset store @@ -616,16 +623,19 @@ def _restore_web_payload(payload: dict[str, Any]) -> SceneNode: # R6.4 translation: legacy node_type "web" -> backend kind # "web_research" (confirmed distinct strings, not a typo). x, y = _position(payload) - node = SceneNode( - id="", x=x, y=y, title="Web Research", kind="web_research", - content=str(payload.get("query", "")), - history=_restore_history(payload.get("conversation_history")), - is_collapsed=bool(payload.get("is_collapsed", False)), - state=WebResearchState( - # ADR-021 stage 21.5: absent in every pre-21.5 row, which is - # exactly the False default Web Research has always behaved as. - research_retain_to_knowledge=bool(payload.get("retain_to_knowledge", False)), + node = with_state( + SceneNode( + id="", x=x, y=y, title="Web Research", kind="web_research", + content=str(payload.get("query", "")), + history=_restore_history(payload.get("conversation_history")), + is_collapsed=bool(payload.get("is_collapsed", False)), + state=WebResearchState( + # ADR-021 stage 21.5: absent in every pre-21.5 row, which is + # exactly the False default Web Research has always behaved as. + research_retain_to_knowledge=bool(payload.get("retain_to_knowledge", False)), + ), ), + WebResearchState, ) research_result = payload.get("research_result") if isinstance(research_result, dict) and research_result: @@ -870,7 +880,7 @@ def _restore_plan_payload(payload: dict[str, Any]) -> SceneNode: goal = str(payload.get("goal", "")) raw_status = str(payload.get("builder_status", "draft") or "draft") status = raw_status if raw_status in _BUILDER_TERMINAL_STATUSES + ("draft",) else "interrupted" - steps = [] + steps: list[dict[str, Any]] = [] for raw in payload.get("steps") or []: if isinstance(raw, dict) and raw.get("title"): steps.append({ @@ -1240,10 +1250,13 @@ def _restore_notes(document: SceneDocument, notes_data: list) -> dict[int, str]: continue try: x, y = _position(note_payload) - note = document.add_note( - x, y, - is_system_prompt=bool(note_payload.get("is_system_prompt", False)), - is_summary_note=bool(note_payload.get("is_summary_note", False)), + note = with_state( + document.add_note( + x, y, + is_system_prompt=bool(note_payload.get("is_system_prompt", False)), + is_summary_note=bool(note_payload.get("is_summary_note", False)), + ), + NoteState, ) document.set_note_content(note.id, str(note_payload.get("content", ""))) # Rows saved before the forced-default fix carry @@ -1308,7 +1321,12 @@ def _restore_charts( chart_payload, "parent_node_id", "parent_node_index", nodes_by_id, all_nodes_map, ) x, y = _position(chart_payload) - chart = document.add_chart_node(x, y, parent_id, chart_type, chart_data, chart_error=chart_error) + chart = with_state( + document.add_chart_node( + x, y, parent_id, chart_type, chart_data, chart_error=chart_error, + ), + ChartState, + ) # Aspect-lock MUST be applied before any resize: resize_chart's # own aspect-preserving re-derivation reads chart_aspect_locked # at call time, and a freshly-created chart always starts locked @@ -1348,7 +1366,7 @@ def _restore_frames( member_ids = [frame_source_map[i] for i in item_indices if i in frame_source_map] if not member_ids: continue - frame = document.create_frame(member_ids) + frame = with_state(document.create_frame(member_ids), FrameState) document.set_group_label(frame.id, str(frame_payload.get("note", "") or "")) document.set_group_color(frame.id, frame_payload.get("color"), frame_payload.get("header_color")) if bool(frame_payload.get("is_locked", True)) != frame.state.is_locked: @@ -1681,7 +1699,7 @@ def _restore_branch_provenance_item_ids( def _restore_pins(document: SceneDocument, pins_data: list) -> None: if not isinstance(pins_data, list) or not pins_data: return - records = [] + records: list[NavigationPinRecord] = [] for pin_payload in pins_data: if not isinstance(pin_payload, dict): continue @@ -1863,12 +1881,14 @@ def _restore_chat_into_document( by_payload_id = dict(nodes_by_id) if isinstance(notes_data, list): for note_index, note_payload in enumerate(notes_data): - note_new_id = notes_map.get(note_index) - if not isinstance(note_payload, dict) or note_new_id is None: + # Not `note_new_id`: that name is bound as a plain str by the + # loop over notes_map.items() earlier in this function. + mapped_note_id = notes_map.get(note_index) + if not isinstance(note_payload, dict) or mapped_note_id is None: continue note_payload_id = note_payload.get("id") if note_payload_id: - by_payload_id[str(note_payload_id)] = note_new_id + by_payload_id[str(note_payload_id)] = mapped_note_id by_payload_id.update(charts_by_id) if not _restore_flat_edges(document, chat_data, by_payload_id): diff --git a/backend/session_save.py b/backend/session_save.py index f000cbb..40c6a3c 100644 --- a/backend/session_save.py +++ b/backend/session_save.py @@ -105,6 +105,25 @@ from typing import Any from backend.canvas import SceneDocument, SceneNode, _content_codec +from backend.domain.node_access import with_state +from backend.domain.node_states import ( + ArtifactState, + ChartState, + ChatState, + CodeReviewState, + CodeSandboxState, + CodeState, + ContainerState, + DocumentState, + FrameState, + GitlinkState, + HarnessState, + HtmlState, + ImageState, + NoteState, + PlanState, + WebResearchState, +) from backend.plugin_sdk import NodeKindSpec, PluginRegistry, discover_plugins from graphlink_settings_store import SettingsManager @@ -217,7 +236,8 @@ def _serialize_history(history: list[dict[str, Any]]) -> list[dict[str, Any]]: # _classify_edges) - these functions never reference `document` or other # nodes at all, mirroring session_load.py's equivalent restorers. -def _serialize_chat_node(node: SceneNode) -> dict[str, Any]: +def _serialize_chat_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, ChatState) if node.state.content_parts is not None: raw_content = _content_codec.process_content_for_serialization(node.state.content_parts) else: @@ -269,11 +289,13 @@ def _serialize_chat_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_code_node(node: SceneNode) -> dict[str, Any]: +def _serialize_code_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, CodeState) return {"node_type": "code", "code": node.state.code, "language": node.state.language} -def _serialize_document_node(node: SceneNode) -> dict[str, Any]: +def _serialize_document_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, DocumentState) return { "node_type": "document", "title": node.title, @@ -290,7 +312,7 @@ def _serialize_document_node(node: SceneNode) -> dict[str, Any]: def _serialize_image_node( - node: SceneNode, document: SceneDocument, asset_store: Any | None = None + raw_node: SceneNode, document: SceneDocument, asset_store: Any | None = None ) -> dict[str, Any]: """ADR-009 stage 9.5: writes the image's bytes to the content-addressed asset store when one is supplied, emitting only a ref - so autosave @@ -303,6 +325,7 @@ def _serialize_image_node( shape, so a chat saved by an older build keeps loading untouched and no row ever has to be rewritten to make this safe. The inline path is what a future cleanup deletes, once no old rows remain in the wild.""" + node = with_state(raw_node, ImageState) asset = document.image_assets.get(node.state.image_asset_id) image_bytes = asset[0] if asset is not None else b"" mime_type = asset[1] if asset is not None else "image/png" @@ -347,7 +370,8 @@ def _serialize_conversation_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_html_node(node: SceneNode) -> dict[str, Any]: +def _serialize_html_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, HtmlState) return { "node_type": "html", "html_content": node.content, @@ -357,7 +381,8 @@ def _serialize_html_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_web_node(node: SceneNode) -> dict[str, Any]: +def _serialize_web_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, WebResearchState) research_result = _camel_to_snake_deep(node.state.research_result) if node.state.research_result else {} return { # R6.5 translation (inverse of R6.4's own): backend kind @@ -374,7 +399,8 @@ def _serialize_web_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_artifact_node(node: SceneNode) -> dict[str, Any]: +def _serialize_artifact_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, ArtifactState) return { "node_type": "artifact", "instruction": node.content, @@ -384,7 +410,8 @@ def _serialize_artifact_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_gitlink_node(node: SceneNode) -> dict[str, Any]: +def _serialize_gitlink_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, GitlinkState) return { "node_type": "gitlink", "task_prompt": node.state.gitlink_task_prompt, @@ -408,12 +435,13 @@ def _serialize_gitlink_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_code_review_node(node: SceneNode) -> dict[str, Any]: +def _serialize_code_review_node(raw_node: SceneNode) -> dict[str, Any]: # NOTE (ADR-002 stage 2.5 gate): every field below is read as # node.state., never via a `state = node.state` alias - # tests/test_node_state_migration.py's bare-attribute ban only # recognizes the `X.state.` shape, so an alias would fail the # build (the _serialize_gitlink_node precedent reads the same way). + node = with_state(raw_node, CodeReviewState) return { "node_type": "code_review", "pr_url": node.state.code_review_pr_url, @@ -456,7 +484,8 @@ def _serialize_code_review_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_code_sandbox_node(node: SceneNode) -> dict[str, Any]: +def _serialize_code_sandbox_node(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, CodeSandboxState) return { "node_type": "code_sandbox", "prompt": node.state.code_sandbox_prompt, @@ -470,7 +499,7 @@ def _serialize_code_sandbox_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_plan_node(node: SceneNode) -> dict[str, Any]: +def _serialize_plan_node(raw_node: SceneNode) -> dict[str, Any]: """ADR-008 stage 8.3: the Builder plan node. NEW-app-only kind (the legacy app never had a Builder) - a legacy load silently skips it, the same documented tolerant behavior every post-legacy kind gets. The @@ -478,6 +507,7 @@ def _serialize_plan_node(node: SceneNode) -> dict[str, Any]: persisted: they describe a RunHandle that cannot survive a restart; session_load's restorer likewise normalizes a non-terminal builder_status to "interrupted" (see PlanState's own docstring).""" + node = with_state(raw_node, PlanState) return { "node_type": "plan", "goal": node.state.plan_goal, @@ -502,7 +532,7 @@ def _serialize_plan_node(node: SceneNode) -> dict[str, Any]: } -def _serialize_harness_node(node: SceneNode) -> dict[str, Any]: +def _serialize_harness_node(raw_node: SceneNode) -> dict[str, Any]: """PLAN-2026-08-24 H1: the harness node. NEW-app-only kind, same tolerant legacy-skip posture as the plan node. Deliberately small: conversation history lives in the workspace transcript, not here (see @@ -510,6 +540,7 @@ def _serialize_harness_node(node: SceneNode) -> dict[str, Any]: plus the two durable identities (workspace id, last run id). session_load normalizes a non-terminal harness_status to "interrupted", the exact PlanState treatment.""" + node = with_state(raw_node, HarnessState) return { "node_type": "harness", "goal": node.state.harness_goal, @@ -637,7 +668,8 @@ def _serialize_plugin_node( } -def _serialize_note(node: SceneNode) -> dict[str, Any]: +def _serialize_note(raw_node: SceneNode) -> dict[str, Any]: + node = with_state(raw_node, NoteState) return { "id": node.id, "content": node.content, @@ -684,7 +716,8 @@ def _serialize_pin(record) -> dict[str, Any]: } -def _serialize_frame(node: SceneNode, frame_source_index: dict[str, int]) -> dict[str, Any]: +def _serialize_frame(raw_node: SceneNode, frame_source_index: dict[str, int]) -> dict[str, Any]: + node = with_state(raw_node, FrameState) item_indices = [frame_source_index[i] for i in node.item_ids if i in frame_source_index] # Technical-debt audit finding: group_width/group_height is the frame's # CURRENT effective size, and reading it unconditionally is CORRECT @@ -735,7 +768,8 @@ def _serialize_frame(node: SceneNode, frame_source_index: dict[str, int]) -> dic } -def _serialize_container(node: SceneNode, all_items_index: dict[str, int]) -> dict[str, Any]: +def _serialize_container(raw_node: SceneNode, all_items_index: dict[str, int]) -> dict[str, Any]: + node = with_state(raw_node, ContainerState) item_indices = [all_items_index[i] for i in node.item_ids if i in all_items_index] width = node.state.group_width if node.state.group_width is not None else 0.0 height = node.state.group_height if node.state.group_height is not None else 0.0 @@ -754,8 +788,9 @@ def _serialize_container(node: SceneNode, all_items_index: dict[str, int]) -> di def _serialize_chart( - node: SceneNode, nodes_index: dict[str, int], parent_id: str | None, + raw_node: SceneNode, nodes_index: dict[str, int], parent_id: str | None, ) -> dict[str, Any]: + node = with_state(raw_node, ChartState) parent_index = nodes_index.get(parent_id) if parent_id is not None else None return { "id": node.id, diff --git a/backend/tests/test_serializer_state_guards.py b/backend/tests/test_serializer_state_guards.py new file mode 100644 index 0000000..fdcfbec --- /dev/null +++ b/backend/tests/test_serializer_state_guards.py @@ -0,0 +1,95 @@ +"""A save never writes a node whose state is not the one its kind promises. + +backend/session_save.py dispatches on `node.kind` and hands the node to a +serializer that reads that kind's state fields directly. Node states are plain, +non-slotted dataclasses, so a node carrying the wrong state - or none at all, +which is what a row written before its kind's state class existed looks like - +does not fail cleanly on its own. It fails partway through building the +payload, with whatever `getattr` happens to hit first. + +That matters more here than anywhere else in the codebase. This is the write +side of persistence: a payload half-built from the wrong fields is a saved +session that cannot be loaded back. + +`node_access.with_state` turns that into one explicit SceneError at the top of +each serializer, before any field is read. These tests pin that for every +serializer that has state to narrow - which is all of them except the thinking +and conversation kinds, whose nodes carry no state at all and whose serializers +touch only SceneNode's own fields. +""" + +from __future__ import annotations + +import pytest + +from backend.canvas import SceneNode +from backend.domain.model import SceneError +from backend.domain.node_states import ChatState +from backend import session_save + + +# (serializer, kind it is registered for, extra positional args after the node) +SERIALIZERS = [ + (session_save._serialize_chat_node, "chat", ()), + (session_save._serialize_code_node, "code", ()), + (session_save._serialize_document_node, "document", ()), + (session_save._serialize_image_node, "image", (None, None)), + (session_save._serialize_html_node, "html", ()), + (session_save._serialize_web_node, "web_research", ()), + (session_save._serialize_artifact_node, "artifact", ()), + (session_save._serialize_gitlink_node, "gitlink", ()), + (session_save._serialize_code_review_node, "code_review", ()), + (session_save._serialize_code_sandbox_node, "code_sandbox", ()), + (session_save._serialize_plan_node, "plan", ()), + (session_save._serialize_harness_node, "harness", ()), + (session_save._serialize_note, "note", ()), + (session_save._serialize_frame, "frame", ({},)), + (session_save._serialize_container, "container", ({},)), + (session_save._serialize_chart, "chart", ({}, None)), +] + +IDS = [fn.__name__ for fn, _, _ in SERIALIZERS] + + +def _node(kind: str, state) -> SceneNode: + return SceneNode(id="n1", x=0.0, y=0.0, title="t", kind=kind, state=state) + + +@pytest.mark.parametrize("serializer, kind, extra", SERIALIZERS, ids=IDS) +def test_a_node_with_no_state_is_refused_not_half_serialized(serializer, kind, extra): + """The shape a pre-migration row takes: the right kind, no state object.""" + with pytest.raises(SceneError): + serializer(_node(kind, None), *extra) + + +@pytest.mark.parametrize("serializer, kind, extra", SERIALIZERS, ids=IDS) +def test_a_node_carrying_another_kinds_state_is_refused(serializer, kind, extra): + """ChatState stands in for "some other kind's state". It is a real state + class with real fields, so a serializer that read from it blindly would + get partway through rather than failing on the first access - which is + exactly the outcome being ruled out.""" + if kind == "chat": + pytest.skip("ChatState is this serializer's own state") + with pytest.raises(SceneError): + serializer(_node(kind, ChatState()), *extra) + + +def test_the_error_names_the_node_and_the_state_it_wanted(): + """A save that fails should say which node and what was missing - the + thing an AttributeError on `.state.gitlink_repo` did not say.""" + with pytest.raises(SceneError) as caught: + session_save._serialize_gitlink_node(_node("gitlink", None)) + message = str(caught.value) + assert "n1" in message + assert "GitlinkState" in message + + +def test_a_correct_node_is_returned_unchanged_and_serializes(): + """The guard is a check, not a transformation: with_state hands back the + same object, so nothing about a valid save changes.""" + node = _node("chat", ChatState(is_user=True)) + node.content = "hello" + payload = session_save._serialize_chat_node(node) + assert payload["node_type"] == "chat" + assert payload["raw_content"] == "hello" + assert payload["is_user"] is True diff --git a/pyproject.toml b/pyproject.toml index 4ed7ee8..d5e951f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -383,8 +383,30 @@ files = [ # TYPE_CHECKING only, and the package now checks clean with no # behavioural change. See that module's own docstring. "settings_store", + # 2026-09-04: the persistence layer, which was 60% of everything left. + # + # session_save.py alone held 246 of the 444 remaining `mypy backend` + # errors, every one the same NodeState | None union-attr. Its per-kind + # serializers are reached through a kind-keyed dispatch table, so each one + # already knows what it is looking at - it just had no way to say so. + # node_access.with_state is that way; the bodies are untouched. + "backend/session_save.py", + "backend/session_load.py", ] ignore_missing_imports = true +# Check the files in `files` fully; use everything they import for type +# information without reporting errors inside it. Without this, listing +# backend/session_save.py pulls its whole import closure - autosave, +# chat_library, assets, api_provider - into the gate and reports 141 errors in +# modules nobody put on the ratchet, which would mean the only way to add a +# clean module is to first clean everything it touches. The ratchet is meant +# to move one module at a time; this is what lets it. +# +# It does not weaken what is already listed: every module in `files` is still +# checked in full, and the followed modules were reporting zero errors of +# their own before this line existed, because the only ones being followed +# were themselves listed. +follow_imports = "silent" [tool.ruff.lint.per-file-ignores] # backend/canvas.py's own docstring: it deliberately re-imports every