Repository navigation
Correct two SceneNode field comments that had gone materially wrong - #400
Merged
Merged
Conversation
`content` and `history` were each documented as belonging to one kind -
chat and conversation respectively - with an explicit "Unused (default)
for every other kind" line. Neither is true, and in `content`'s case a
later line in the SAME comment block already contradicted the earlier one.
Measured against the SceneNode(...) constructions in
backend/domain/graph.py and backend/session_load.py:
content 10 kinds: artifact, chat, document, harness, html, image,
note, plan, thinking, web_research
history 7 kinds: artifact, chat, code_sandbox, conversation, gitlink,
html, web_research
This matters beyond tidiness. Those two fields are the largest part of
what makes SceneNode look like an unfinished ADR-002 stage 2.5 migration -
"kind-specific fields still sitting on the shared node". They are not.
They are shared fields, and moving them into per-kind state classes would
mean duplicating one field across ten classes and another across seven,
which is worse than what is there now. The comments were the evidence a
reader would use to decide that, and they said the opposite.
backend/domain/node_states.py's docstring turns out to be right where I
first read it as wrong: listing `conversation` among the kinds with no
state class is correct, because its history is not a kind-specific field.
A gate pins both lists. That is the cheap answer to a shape this codebase
keeps hitting - a comment asserts a closed set, the set grows, nothing
fails. backend/api/_shared.py names its two callers by hand and was wrong
within one commit of being written; these two were wrong for longer. If a
new kind starts using either field the build now fails, naming both the
set and the comment to update.
Test plan:
- 3 new gate tests: both kind sets derived by AST from the two
node-creating modules, plus a guards-the-guard check so a renamed class
or changed construction style cannot make them compare empty sets.
- Verified the gate fails when the sets drift: the first version was
written from a graph.py-only scan and failed against reality, which is
how the two missing kinds (artifact, web_research) were found.
- Full suite: 3164 passed, 19 skipped. ruff clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dovvnloading
added a commit
that referenced
this pull request
Sep 4, 2026
Both shipped earlier today with the defect still reachable. A verification pass measured them; neither was theoretical. Budget caps could still restore unusable. #393's commit message claimed the plan's caps "keep falling back to their real defaults rather than zero, since a restored plan with max_steps=0 could never run again". They did not - _non_negative_int is `max(0, int(value))` with a fallback only when int() RAISES, so every numeric route to zero went straight through, and the outer `or default` only catches Python-falsy values: max_steps=-5 -> 0 max_steps='0' -> 0 max_steps='-5'-> 0 max_steps=0.5 -> 0 The harness restorer was worse: its local `_int` helper passed negatives through unchanged, so a saved -5 restored as -5 and a saved '0' as 0. builder._spend_breach then rejects the node on its first tick - it sits on the canvas and can never run again. _positive_int is the right helper for a cap, where zero is not a value, and both restorers now use it. Counters keep _non_negative_int, because 0 is a real count. The harness's local helper is gone in favour of the two shared ones. The field-comment gate could not see the modules it was written to cover. #400 corrected `content`'s comment to "TEN kinds" and added a gate to pin it. Both were wrong: backend/domain/groups.py builds `kind="frame"` and `kind="container"` nodes with `content=`, and the gate hard-coded graph.py and session_load.py while its own docstring called them "the two modules that create nodes". Twelve kinds, not ten - and a thirteenth landing in groups.py would have left the gate green. That is the failure the gate exists to prevent, reproduced inside the gate. It now DISCOVERS every module under backend/ that constructs a SceneNode instead of naming them, with a test asserting discovery finds more than the original two. The kind sets stay hand-authored on purpose: a human deciding "yes, a thirteenth kind should write content" is the checkpoint. Test plan: - 9 parametrised plan-cap cases and 6 harness-cap cases covering every numeric and non-numeric route to zero or negative, plus a case pinning that a real cap survives and a counter of 0 is not rewritten. - A discovery test that fails if the module scan narrows back. - tests/test_node_state_migration.py caught the first draft of these tests aliasing node.state; rewritten to the required X.state.<field> chain rather than weakening the gate. - Full suite: 3181 passed, 19 skipped. ruff clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
dovvnloading
added a commit
that referenced
this pull request
Sep 4, 2026
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>
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
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
contentandhistorywere each documented as belonging to one kind — chat and conversation respectively — with an explicit "Unused (default) for every other kind" line. Neither is true. Incontent's case a later line in the same comment block already contradicted the earlier one.Measured against the
SceneNode(...)constructions inbackend/domain/graph.pyandbackend/session_load.py:contenthistoryThis matters beyond tidiness, and it corrected my own reading of the code. Those two fields are the bulk of what makes
SceneNodelook like an unfinished ADR-002 stage 2.5 migration — "kind-specific fields still sitting on the shared node". They are not kind-specific. Moving them into per-kind state classes would mean duplicating one field across ten classes and another across seven, which is strictly worse than what is there now. The comments were exactly the evidence a reader would use to make that call, and they said the opposite.backend/domain/node_states.py's docstring turns out to be right where I first read it as wrong: listingconversationamong the kinds with no state class is correct, because its history is not a kind-specific field — it is this shared one.Change
Both comments now describe reality, say why the field stayed on
SceneNode, and point at the gate that keeps them honest.A gate pins both lists. That is the cheap answer to a shape this codebase keeps hitting — a comment asserts a closed set, the set grows, and nothing fails.
backend/api/_shared.pynames its two callers by hand and was wrong within one commit of being written; these two were wrong for considerably longer. If a new kind starts using either field, the build now fails and names both the set and the comment to update.Test plan
graph.py-only scan and failed against reality, which is how the two missing kinds (artifact,web_research) were found in the first place.ruff check .clean.What this does not do
This is a small, honest slice of QA item 9, not the item. The genuine remaining debt there is
SceneDocument— 144 public members, 79 of them node-kind-specific — which is a multi-PR architectural change, not something to attach to a comment fix. See the QA document's own work log.🤖 Generated with Claude Code