diff --git a/backend/domain/model.py b/backend/domain/model.py index 9abf61d..f2b9b57 100644 --- a/backend/domain/model.py +++ b/backend/domain/model.py @@ -163,15 +163,17 @@ class SceneNode: # It is NOT chat-only and has not been for a long time. This comment used # to end "Unused (default) for every other kind" while a later line in the # same block already contradicted it, and the list has kept growing since. - # Verified 2026-09-04 against the SceneNode(...) constructions in - # backend/domain/graph.py and backend/session_load.py, TEN kinds populate - # it: artifact, chat, document, harness, html, image, note, plan, - # thinking, web_research. Pinned by tests/test_shared_node_field_docs.py, - # so this list fails the build rather than rotting again. + # TWELVE kinds populate it: artifact, chat, container, document, frame, + # harness, html, image, note, plan, thinking, web_research. Pinned by + # tests/test_shared_node_field_docs.py, which DISCOVERS the modules that + # construct a SceneNode rather than naming them - an earlier revision of + # this comment said TEN because the gate hard-coded graph.py and + # session_load.py and never looked at groups.py, where the frame and + # container nodes are built. # # That shared use is deliberate, and it is why `content` did not move to a - # per-kind class in the ADR-002 stage 2.5 migration: a field ten kinds - # write would have to be duplicated across ten state classes to live + # per-kind class in the ADR-002 stage 2.5 migration: a field twelve kinds + # write would have to be duplicated across twelve state classes to live # there, which is worse than one core field. Treat it as core, like # title - not as a kind-specific leftover. content: str = "" @@ -190,7 +192,7 @@ class SceneNode: # It is NOT conversation-only and has not been since the plugin kinds # landed. This comment used to say "Unused (default empty list) for every # other kind"; verified 2026-09-04 against the restorers in - # backend/domain/graph.py and backend/session_load.py, SEVEN kinds + # every module under backend/ that builds a SceneNode, SEVEN kinds # populate it: artifact, chat, code_sandbox, conversation, gitlink, html, # web_research. Any kind that holds a back-and-forth with a model keeps it # here. Pinned by tests/test_shared_node_field_docs.py. diff --git a/backend/session_load.py b/backend/session_load.py index 1a26706..5864b5e 100644 --- a/backend/session_load.py +++ b/backend/session_load.py @@ -386,6 +386,32 @@ def _non_negative_int(value: Any, default: int = 0) -> int: return default +def _positive_int(value: Any, default: int) -> int: + """_non_negative_int's sibling for BUDGET CAPS, where zero is not a value. + + A counter may legitimately be 0; a cap may not. `max_steps=0`, + `max_tokens=0` or `max_wall_seconds=0` restores a plan that + builder._spend_breach rejects on its first tick - the plan is on the + canvas and can never run again. + + _non_negative_int is the wrong helper for these and was used for them + anyway. It only falls back when `int()` RAISES, so every numeric route to + zero got through it: + + max_steps=-5 -> 0 max_steps='0' -> 0 + max_steps='-5' -> 0 max_steps=0.5 -> 0 + + and the outer `or default` at the call site only catches Python-falsy + values, so the string "0" and "-5" sail past that too. Anything that does + not resolve to a usable positive cap falls back to the documented + default here.""" + try: + parsed = int(value) + except (TypeError, ValueError): + return default + return parsed if parsed > 0 else default + + def _position(payload: dict[str, Any]) -> tuple[float, float]: position = payload.get("position") if isinstance(position, dict): @@ -890,9 +916,9 @@ def _restore_plan_payload(payload: dict[str, Any]) -> SceneNode: builder_status=status, builder_mode=mode if mode in ("copilot", "autopilot") else "copilot", builder_run_id=str(payload.get("builder_run_id", "")), - builder_max_steps=_non_negative_int(payload.get("max_steps") or 12, 12), - builder_max_tokens=_non_negative_int(payload.get("max_tokens") or 150_000, 150_000), - builder_max_wall_seconds=_non_negative_int(payload.get("max_wall_seconds") or 900, 900), + builder_max_steps=_positive_int(payload.get("max_steps"), 12), + builder_max_tokens=_positive_int(payload.get("max_tokens"), 150_000), + builder_max_wall_seconds=_positive_int(payload.get("max_wall_seconds"), 900), builder_spent_steps=_non_negative_int(payload.get("spent_steps")), builder_spent_tokens=_non_negative_int(payload.get("spent_tokens")), builder_spent_wall_seconds=_non_negative_int(payload.get("spent_wall_seconds")), @@ -935,12 +961,6 @@ def _restore_harness_payload(payload: dict[str, Any]) -> SceneNode: "elapsedMs": elapsed_ms, }) - def _int(key: str, default: int) -> int: - try: - return int(payload.get(key, default) or default) - except (TypeError, ValueError): - return default - return SceneNode( id="", x=x, y=y, title=f"Agent: {goal[:40]}" if goal else "Agent", @@ -959,12 +979,12 @@ def _int(key: str, default: int) -> int: harness_workspace_id=str(payload.get("workspace_id") or uuid.uuid4().hex[:12]), harness_workspace_path=str(payload.get("workspace_path", "")), harness_activity=activity, - harness_max_turns=_int("max_turns", 16), - harness_spent_turns=_int("spent_turns", 0), - harness_spent_tokens=_int("spent_tokens", 0), - harness_context_tokens=_int("context_tokens", 0), - harness_max_context_tokens=_int("max_context_tokens", 48_000), - harness_compactions=_int("compactions", 0), + harness_max_turns=_positive_int(payload.get("max_turns"), 16), + harness_spent_turns=_non_negative_int(payload.get("spent_turns")), + harness_spent_tokens=_non_negative_int(payload.get("spent_tokens")), + harness_context_tokens=_non_negative_int(payload.get("context_tokens")), + harness_max_context_tokens=_positive_int(payload.get("max_context_tokens"), 48_000), + harness_compactions=_non_negative_int(payload.get("compactions")), ), is_collapsed=bool(payload.get("is_collapsed", False)), ) diff --git a/backend/tests/test_session_load.py b/backend/tests/test_session_load.py index 4352de1..7a180b4 100644 --- a/backend/tests/test_session_load.py +++ b/backend/tests/test_session_load.py @@ -10,6 +10,8 @@ approximated. """ +import pytest + import backend.agents as agents_module # noqa: F401 - see test_canvas.py's own import-order note from backend.canvas import SceneDocument from backend.session_load import restore_chat_into_document @@ -112,6 +114,56 @@ def test_code_review_node_survives_non_numeric_counts(): assert node.state.code_review_quality_score == 0 +@pytest.mark.parametrize("bad_cap", [-5, "-5", "0", 0.5, 0, "twelve", None, "", []]) +def test_plan_caps_never_restore_to_an_unusable_value(bad_cap): + """A counter may be 0; a cap may not. builder._spend_breach rejects a plan + with max_steps=0 on its first tick, so it lands on the canvas and can never + run again. + + _non_negative_int was the wrong helper for these and was used anyway: it + only falls back when int() RAISES, so every numeric route to zero got + through - -5 and '0' and 0.5 all clamped to 0 rather than falling back.""" + document = _restore(nodes=[{ + "node_type": "plan", "position": {"x": 0.0, "y": 0.0}, "goal": "g", + "max_steps": bad_cap, "max_tokens": bad_cap, "max_wall_seconds": bad_cap, + }]) + node = next(n for n in document.nodes.values() if n.kind == "plan") + assert node.state.builder_max_steps == 12 + assert node.state.builder_max_tokens == 150_000 + assert node.state.builder_max_wall_seconds == 900 + + +@pytest.mark.parametrize("bad_cap", [-5, "-5", "0", 0, "x", None]) +def test_harness_caps_never_restore_negative_or_zero(bad_cap): + """The harness restorer had it worse than plan: its local `_int` helper + passed negatives straight through, so a saved -5 restored as -5.""" + document = _restore(nodes=[{ + "node_type": "harness", "position": {"x": 0.0, "y": 0.0}, "goal": "g", + "max_turns": bad_cap, "max_context_tokens": bad_cap, + }]) + node = next(n for n in document.nodes.values() if n.kind == "harness") + assert node.state.harness_max_turns == 16 + assert node.state.harness_max_context_tokens == 48_000 + + +def test_real_caps_and_zero_counters_both_survive(): + """The other side: a legitimate cap is kept, and a counter of 0 - which is + a real value, not a missing one - is not rewritten to a default.""" + document = _restore(nodes=[ + {"node_type": "plan", "position": {"x": 0.0, "y": 0.0}, "goal": "g", + "max_steps": 7, "spent_steps": 0}, + {"node_type": "harness", "position": {"x": 0.0, "y": 0.0}, "goal": "g", + "max_turns": 9, "spent_turns": 0, "compactions": 0}, + ]) + plan = next(n for n in document.nodes.values() if n.kind == "plan") + harness = next(n for n in document.nodes.values() if n.kind == "harness") + assert plan.state.builder_max_steps == 7 + assert plan.state.builder_spent_steps == 0 + assert harness.state.harness_max_turns == 9 + assert harness.state.harness_spent_turns == 0 + assert harness.state.harness_compactions == 0 + + def test_plan_node_survives_non_numeric_budget_fields(): """The same bug class in the plan restorer: it already guarded activity elapsedMs against exactly this, then read six budget fields with a bare diff --git a/tests/test_shared_node_field_docs.py b/tests/test_shared_node_field_docs.py index b27b0a9..ef8e073 100644 --- a/tests/test_shared_node_field_docs.py +++ b/tests/test_shared_node_field_docs.py @@ -4,7 +4,7 @@ backend/domain/node_states.py, and both were documented as belonging to one kind - `content` to chat, `history` to conversation - with an explicit "Unused for every other kind" line. Neither was true any more. A 2026-09-04 -audit measured ten kinds writing `content` and seven writing `history`, +audit measured twelve kinds writing `content` and seven writing `history`, and in `content`'s case a later line in the SAME comment block already contradicted the earlier one. @@ -17,8 +17,9 @@ list to be right. Derived from the kinds that actually construct a SceneNode with each field, -across the two modules that create nodes: backend/domain/graph.py (live -creation) and backend/session_load.py (restore from a saved chat). +across every module under backend/ that does so - DISCOVERED, not listed. +Hard-coding that list is how the first version of this gate shipped with the +wrong answer. Same posture as tests/undo_classification.py: a hand-authored expectation plus a gate that fails when the code and the expectation diverge. If this @@ -33,31 +34,61 @@ from pathlib import Path REPO_ROOT = Path(__file__).resolve().parents[1] -NODE_CREATING_MODULES = ( - REPO_ROOT / "backend" / "domain" / "graph.py", - REPO_ROOT / "backend" / "session_load.py", -) +BACKEND = REPO_ROOT / "backend" # Hand-authored, and deliberately duplicated in backend/domain/model.py's own # field comments. The gate exists to keep the two copies honest. CONTENT_KINDS = { - "artifact", "chat", "document", "harness", "html", - "image", "note", "plan", "thinking", "web_research", + "artifact", "chat", "container", "document", "frame", "harness", + "html", "image", "note", "plan", "thinking", "web_research", } HISTORY_KINDS = {"artifact", "chat", "code_sandbox", "conversation", "gitlink", "html", "web_research"} +def _node_creating_modules() -> list[Path]: + """Every module under backend/ that constructs a SceneNode, DISCOVERED. + + The first version of this gate hard-coded two paths - graph.py and + session_load.py - and its own docstring called them "the two modules that + create nodes". They were not. groups.py builds `kind="frame"` and + `kind="container"` nodes with `content=`, so the list this gate was + written to pin was itself wrong by two kinds, and injecting a thirteenth + into groups.py would have left the gate green. + + That is precisely the failure this file exists to prevent - a closed set + asserted by hand, growing, with nothing failing - reproduced one level up + in the guard itself. Discovering the modules removes the hand-authored + half of the problem; the kind sets below stay hand-authored on purpose, + because a human deciding "yes, a thirteenth kind should write content" is + the checkpoint. + """ + modules: list[Path] = [] + for path in sorted(BACKEND.rglob("*.py")): + if "tests" in path.parts or "__pycache__" in path.parts: + continue + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: # pragma: no cover - a broken module fails elsewhere + continue + for node in ast.walk(tree): + if isinstance(node, ast.Call) and _callee_name(node) == "SceneNode": + modules.append(path) + break + return modules + + +def _callee_name(call: ast.Call) -> str | None: + func = call.func + return func.id if isinstance(func, ast.Name) else getattr(func, "attr", None) + + def _kinds_constructing_with(field_name: str) -> set[str]: """Kinds passed to a SceneNode(...) call that also passes `field_name`.""" kinds: set[str] = set() - for path in NODE_CREATING_MODULES: + for path in _node_creating_modules(): tree = ast.parse(path.read_text(encoding="utf-8")) for node in ast.walk(tree): - if not isinstance(node, ast.Call): - continue - func = node.func - name = func.id if isinstance(func, ast.Name) else getattr(func, "attr", None) - if name != "SceneNode": + if not isinstance(node, ast.Call) or _callee_name(node) != "SceneNode": continue passed = {kw.arg for kw in node.keywords if kw.arg} if field_name not in passed: @@ -68,6 +99,13 @@ def _kinds_constructing_with(field_name: str) -> set[str]: return kinds +def test_the_module_discovery_finds_more_than_the_two_originally_hard_coded(): + """The specific miss. If discovery ever narrows back to graph.py and + session_load.py, the kind sets above stop being trustworthy.""" + found = {p.name for p in _node_creating_modules()} + assert {"graph.py", "session_load.py", "groups.py"} <= found, found + + def test_content_is_written_by_the_kinds_its_comment_names(): assert _kinds_constructing_with("content") == CONTENT_KINDS