Narrow node state at the access point, and put mypy on backend/domain - #410
Merged
Conversation
The QA audit called this blocked, and #397 and #407 both repeated the claim: a typed accessor for SceneNode.state was said to be impossible because tests/test_node_state_migration.py rejects the alias one needs. That was wrong, and re-reading the gate is what showed it. Its own _is_via_state checks only that a migrated field sits on something whose attribute is `state` - it constrains the ACCESS SHAPE, `<anything>.state.<field>`, and says nothing about how the node was obtained. So narrowing the NODE is permitted where aliasing the STATE is not, and `node.state.code_review_pr_url` satisfies the gate and the type checker at once. require_node(nodes, node_id, kind, StateClass) returns the node with its state type narrowed, and does the two existing SceneError raises verbatim. optional_node is its silent sibling for the fail_*_run methods. Eighteen copies of the same five-line preamble collapse to one call each. mypy backend: 845 errors -> 659 nodes_code_review.py + nodes_gitlink.py: 93 -> 0 Both now check clean, so [tool.mypy].files gains its first three backend/domain/ entries. The rest of the package follows as its per-kind groups are extracted the same way (#409). TWO DELIBERATE BEHAVIOUR CHANGES, both narrowing a crash into a handled error. Five methods had no kind check at all: three gitlink ones that raised only on a missing node, and two fail_*_run ones that returned None only for a missing node. Passing a wrong-kind node id to any of them reached `node.state.<kind>_<field>` on a state class without that field and raised AttributeError - which the WS layer does not translate, unlike SceneError. They now behave like every sibling: raise SceneError, or (for fail_*_run, whose documented contract is already "silent when the node has gone") return None. Test plan: - 2 new tests pinning both changes: a wrong-kind id raises SceneError from store/fetch/append, and fail_*_run is a quiet no-op for it. - tests/test_node_state_migration.py and test_domain_purity.py green - the access shape is unchanged and node_access.py imports nothing the domain layer may not. - Full suite: 3226 passed, 19 skipped. ruff clean, mypy clean across 33 source files (was 30). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dovvnloading
added a commit
that referenced
this pull request
Sep 4, 2026
The five mixins the previous commit created reported 92 mypy errors, every one the same union-attr shape: SceneNode.state is typed `NodeState | None` against a field-less marker, so `node.state.research_stage` cannot be verified. All 92 sat in exactly 17 methods, and all 17 obtain their node with the same hand-written preamble require_node/optional_node (backend/domain/node_access.py, PR #410) already replaces. Converted; the five modules now check clean. Nine of the seventeen were a straight swap - they already checked the kind and raised the same two messages require_node raises. The other eight are a behaviour change and should be read as one. The completion and failure paths for web_research, artifact and code_sandbox looked their node up WITHOUT checking its kind, on the reasoning - written into two of the docstrings - that the id had already been validated earlier in the same request by the matching start_* call. That is true, and it is still true: the check is redundant on every live path. It is also exactly why the gap was invisible. Node states are plain, non-slotted dataclasses, so `node.state.research_stage = "completed"` against a chat node does not fail; it grafts a phantom attribute onto ChatState and returns happily. backend/tests/test_wrong_kind_node_guards.py pins the new contract for all eight, plus complete_gitlink_run and complete_gitlink_apply, which gained the same check in #409 with nothing asserting it. Each case is checked three ways: the wrong-kind call raises (or, for the fail_* methods, returns None, matching their documented silence when the node has gone), and the wrong-kind node's state is left byte-identical. Verified to fail when a guard is removed. One error message changed: set_html_splitter_state said "node is not an html node" and now says "a html node", the wording every other kind produces. Nothing in the repo or the client reads it. [tool.mypy].files widens from 3 backend/domain/ modules to 13 of 18. The five left out are graph.py and the four cross-cutting mixins (groups/branches/commands/layout), which hold 124 errors of the same shape - they read per-kind state off nodes they look up generically, so require_node has no kind to narrow to. Recorded as the next piece of work rather than waved through. Test plan: full suite, 3,244 passed / 19 skipped. Two unrelated subprocess-timing tests (test_mcp_client stdin-drain, test_execution_guard grandchild-kill) failed under load in that run and pass on their own; neither imports backend.domain. ruff clean; mypy clean across 42 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dovvnloading
added a commit
that referenced
this pull request
Sep 4, 2026
* Finish lifting the per-kind node methods out of SceneDocument PR #409 moved the two largest per-kind method groups (code_review, gitlink) into their own mixins and left the other thirteen kinds behind. This moves those thirteen, which is the rest of that job: 42 members and 983 lines out of graph.py, taking it from 2,591 lines to 1,502. Five new mixins, grouped by what the kinds actually have in common rather than one module per kind: nodes_agent_runs.py AgentRunOps web_research, artifact, code_sandbox - the three kinds with a start/progress/complete/ fail run lifecycle nodes_planning.py PlanningOps plan, harness nodes_conversational.py ConversationalOps chat, conversation nodes_content.py ContentOps document, thinking, html, note nodes_visual.py VisualOps chart, image Every one of the 42 moved members was verified AST-identical to its pre-move version (ast.unparse round-trip against `git show HEAD:backend/domain/graph.py`), verified absent from SceneDocument's own body afterwards, verified reachable on SceneDocument, and verified to be defined exactly once across the whole MRO. Methods are regrouped by kind inside each new module instead of keeping the order that successive increments happened to append them in; nothing else changed. Two supporting edits: SceneDocumentParts gains `adopt_pending_system_prompt`, which add_chat_node calls and which stays on SceneDocument, and its `add_chat_node` declaration is tightened from a `*args: Any, **kwargs: Any` hedge to the real signature. The hedge was fine while the body lived in SceneDocument itself; now that ConversationalOps defines it, an inexact declaration in a base class is an incompatible-override error rather than a harmless fiction. #409 also left two section-header comments in graph.py describing methods it had just moved away, plus 21 stray blank lines where they used to sit. The headers carried real information about those kinds' import posture, so that text moves into the nodes_gitlink.py and nodes_code_review.py module docstrings rather than being dropped. Test plan: full suite, 3,226 passed / 19 skipped. ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Narrow the five new mixins, and widen mypy onto most of backend/domain The five mixins the previous commit created reported 92 mypy errors, every one the same union-attr shape: SceneNode.state is typed `NodeState | None` against a field-less marker, so `node.state.research_stage` cannot be verified. All 92 sat in exactly 17 methods, and all 17 obtain their node with the same hand-written preamble require_node/optional_node (backend/domain/node_access.py, PR #410) already replaces. Converted; the five modules now check clean. Nine of the seventeen were a straight swap - they already checked the kind and raised the same two messages require_node raises. The other eight are a behaviour change and should be read as one. The completion and failure paths for web_research, artifact and code_sandbox looked their node up WITHOUT checking its kind, on the reasoning - written into two of the docstrings - that the id had already been validated earlier in the same request by the matching start_* call. That is true, and it is still true: the check is redundant on every live path. It is also exactly why the gap was invisible. Node states are plain, non-slotted dataclasses, so `node.state.research_stage = "completed"` against a chat node does not fail; it grafts a phantom attribute onto ChatState and returns happily. backend/tests/test_wrong_kind_node_guards.py pins the new contract for all eight, plus complete_gitlink_run and complete_gitlink_apply, which gained the same check in #409 with nothing asserting it. Each case is checked three ways: the wrong-kind call raises (or, for the fail_* methods, returns None, matching their documented silence when the node has gone), and the wrong-kind node's state is left byte-identical. Verified to fail when a guard is removed. One error message changed: set_html_splitter_state said "node is not an html node" and now says "a html node", the wording every other kind produces. Nothing in the repo or the client reads it. [tool.mypy].files widens from 3 backend/domain/ modules to 13 of 18. The five left out are graph.py and the four cross-cutting mixins (groups/branches/commands/layout), which hold 124 errors of the same shape - they read per-kind state off nodes they look up generically, so require_node has no kind to narrow to. Recorded as the next piece of work rather than waved through. Test plan: full suite, 3,244 passed / 19 skipped. Two unrelated subprocess-timing tests (test_mcp_client stdin-drain, test_execution_guard grandchild-kill) failed under load in that run and pass on their own; neither imports backend.domain. ruff clean; mypy clean across 42 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The audit was wrong to call this blocked
QA item C2 gap 1 — 327
union-attrerrors fromSceneNode.statebeing typedNodeState | Noneagainst a field-less marker — was written off as impossible, and #397 and #407 both repeated the claim: a typed accessor needs a local alias, andtests/test_node_state_migration.pyrejects aliases.Re-reading the gate is what showed that was wrong. Its own
_is_via_state:It constrains the access shape —
<anything>.state.<field>— and says nothing about how the node was obtained. So narrowing the node is permitted where aliasing the state is not, andnode.state.code_review_pr_urlsatisfies the gate and the type checker at the same time.Change
require_node(nodes, node_id, kind, StateClass)returns the node with its state type narrowed, performing the two existingSceneErrorraises verbatim.optional_nodeis its silent sibling for thefail_*_runmethods. Eighteen copies of the same five-line preamble collapse to one call each.Both modules now check clean, so
[tool.mypy].filesgains its first threebackend/domain/entries. The rest of the package follows as its per-kind groups are extracted the same way (#409)._NodeWithexists only for the checker — at runtimerequire_nodereturns the plainSceneNodeit looked up, unchanged.Two deliberate behaviour changes
Both narrow a crash into a handled error, and both are pinned by new tests.
Five methods had no kind check at all: three gitlink ones that raised only on a missing node, and two
fail_*_runones that returnedNoneonly for a missing node. Passing a wrong-kind node id to any of them reachednode.state.<kind>_<field>on a state class without that field and raisedAttributeError— which the WS layer does not translate, unlikeSceneError.They now behave like every sibling: raise
SceneError, or — forfail_*_run, whose documented contract is already "silent when the node has gone" — returnNone.Test plan
SceneErrorfrom store/fetch/append, andfail_*_runis a quiet no-op for it.tests/test_node_state_migration.pyandtest_domain_purity.pygreen — the access shape is unchanged, andnode_access.pyimports nothing the domain layer may not.ruffclean,mypyclean across 33 source files (was 30).🤖 Generated with Claude Code