Lift the two largest per-kind method groups out of SceneDocument - #409
Merged
Conversation
SceneDocument had 144 public members, 79 of them specific to a single node kind, in a 3,037-line module. That is most of what makes adding a node kind a 44-file change, and it is the half of QA item C3 that is real - the other half, "kind-specific fields left on SceneNode", turned out on measurement to be shared fields that belong there (see #400, #402). backend/domain/ already decomposes its CROSS-CUTTING concerns this way: SceneDocument is composed from BranchOps, GroupOps, LayoutOps and CommandOps. This extends the same idiom to the PER-KIND ones, starting with the two largest groups: backend/domain/nodes_code_review.py CodeReviewOps 9 methods backend/domain/nodes_gitlink.py GitlinkOps 11 methods graph.py 3,037 -> 2,591 lines methods in graph.py 93 -> 73 SceneDocument public API 144 -> 144 (unchanged, by design) Both mixins need only what SceneDocumentParts already declares - nodes, connect, _counter - so they inherit it and stay type-checkable in isolation like their cross-cutting siblings, and tests/test_mixin_declaration_bases.py covers them automatically through the MRO rather than by being told about them. This is a relocation, not a rewrite. Every one of the 20 moved methods was verified AST-identical to its pre-move version (ast.unparse round-trip compared against `git show HEAD:backend/domain/graph.py`), and every one is still reachable on SceneDocument. store_code_review_diff's own _bundle_int helper moved with the methods that use it. Two kinds, not all fifteen: this proves the extraction on the largest groups and leaves the pattern for the rest to follow one at a time. The remaining thirteen are 13-128 lines each. Test plan: - 20/20 moved methods AST-identical; MRO is SceneDocument -> Branch/Group/Layout/Command/CodeReview/Gitlink Ops -> SceneDocumentParts -> object. - tests/test_domain_purity.py green: neither new module imports anything the domain layer may not. - Full suite: 3224 passed, 19 skipped. ruff and mypy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dovvnloading
added a commit
that referenced
this pull request
Sep 4, 2026
…#410) 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.
Problem
SceneDocumenthad 144 public members, 79 of them specific to a single node kind, in a 3,037-line module. That is most of what makes adding a node kind a 44-file change — the headline cost in QA item C3.It is also the half of C3 that survived scrutiny. The other half — "kind-specific fields left on
SceneNodeby an unfinished migration" — turned out on measurement to be shared fields that belong there (contentacross 12 kinds,historyacross 7); see #400 and #402, which corrected that claim rather than acting on it.Change
backend/domain/already decomposes its cross-cutting concerns this way:SceneDocumentis composed fromBranchOps,GroupOps,LayoutOpsandCommandOps. This extends the same idiom to the per-kind ones, starting with the two largest groups:backend/domain/nodes_code_review.pyCodeReviewOpsbackend/domain/nodes_gitlink.pyGitlinkOpsBoth mixins need only what
SceneDocumentParts(#407) already declares —nodes,connect,_counter— so they inherit it and stay type-checkable in isolation like their cross-cutting siblings.tests/test_mixin_declaration_bases.pypicks them up automatically through the MRO rather than by being told about them.This is a relocation, not a rewrite
Every one of the 20 moved methods was verified AST-identical to its pre-move version —
ast.unparseround-trip compared againstgit show HEAD:backend/domain/graph.py— and every one is still reachable onSceneDocument:store_code_review_diff's own_bundle_inthelper moved with the methods that use it.Two kinds, not all fifteen. This proves the extraction on the largest groups and leaves the pattern for the rest to follow one at a time; the remaining thirteen are 13–128 lines each. Doing all of them in one sweep is the big-bang cleanup this repo's ADRs explicitly reject.
Test plan
tests/test_domain_purity.pygreen — neither new module imports anything the domain layer may not.ruffandmypyclean.🤖 Generated with Claude Code