Skip to content

Let node commands address loops inside a composite by their own id - #223

Merged
scgopi merged 3 commits into
mainfrom
fix/217-subgraph-addressing
Aug 30, 2026
Merged

scgopi merged 3 commits into
mainfrom
fix/217-subgraph-addressing

Conversation

@scgopi

@scgopi scgopi commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Fixes item 15 of #217 (sub-graph children have no memo/playbook path from the CLI).

Problem

A loop created with --into <composite> lives in a nested GraphStore on the composite node, but every node command except node create --into resolved its target against the top-level graph's nodes only. A piloted child following its briefing (graphcode node memo <project> <its-own-id> …) got no loop <id> in this graph; same for node refine, node send, node delete, node update, and edge create. Sub-graph children could only be managed in the app.

Fix

  • GraphStore.handle now auto-routes any command whose target id names no top-level loop but exists inside a sub-graph: it is wrapped as subGraphCommand for the composite that holds it and dispatched through the existing runInSubGraph machinery. Each store level descends one hop, so arbitrarily nested composites need no path form — ids are already unique across the whole tree (LoopGraph.containsAtAnyDepth's documented rule). edge create routes only when both endpoints share one sub-graph; anything else falls through to the existing guards.
  • Commands naming an id that exists nowhere still refuse with the messages callers expect.
  • Refusals raised inside a sub-graph store were previously said to nobody (the ephemeral child owns no connections) — runInSubGraph now buffers them and re-announces on the parent before its graphChanged broadcast, so a one-shot CLI client sees the error instead of timing out.
  • Help text documents that child ids are accepted as-is paired with the project path.

Tests

New SubGraphAddressingTests (8 tests): memo/refine/send/delete/edge-create addressed to a child by its own id, a nested child reached through both levels, unknown-id refusal, and in-sub-graph refusal forwarding. Test run with 1301 tests in 141 suites passed; swiftlint clean, swift-format clean on touched files.

Node memo/refine/send/delete (and update, rename, promote, stop, edge
create) resolved their target against the top-level graph's nodes only,
so a loop created with --into got "no loop <id> in this graph" when it
memoed or refined itself per its briefing, and sub-graph children could
only be managed in the app. A command whose target names no top-level
loop but lives inside a sub-graph is now wrapped for the composite that
holds it and dispatched through the existing runInSubGraph descent, one
hop per store level. Refusals raised inside a sub-graph store are now
re-announced on the parent's connections instead of being said to
nobody. (issue #217, item 15)
@scgopi

scgopi commented Aug 30, 2026

Copy link
Copy Markdown
Owner Author

Review verdict: ✅ Approve

(Formal approval is blocked because this account authored the PR, so the verdict is recorded here as a comment. Reviewed against #217 item 15 with a full build + test run on fix/217-subgraph-addressing @ 52c731b.)

Verification

Check Result
Build (xcodebuild, macOS) SUCCEEDED
Full test suite 1301 tests / 141 suites passed — matches the PR claim
New SubGraphAddressingTests all 8 ran and passed (individually verified in the test log)
swiftlint 0 errors
swift-format touched files clean (DaemonBootstrap.swift failures pre-exist on main, untouched here)
Merge base origin/main is only a version bump ahead — no conflict risk

Correctness against the bug

The reported failure — child loops getting no loop <id> in this graph for memo/refine/send/delete/update/edge create — is fixed at the right layer: routeIntoSubGraph resolves the top-level composite owner via containsAtAnyDepth (recursion confirmed at LoopGraph.swift:62) and wraps for the existing runInSubGraph descent, so nested composites work with no new path form (covered by aNestedChildIsReachedThroughBothLevelsByItsOwnId). Ids that exist nowhere still fall through to the original guards with unchanged messages, and the edge rule (both endpoints must share one composite) is structurally right — an edge can only live in the graph holding both endpoints, so cross-depth/cross-composite pairs are correctly still refused.

The error-forwarding fix is validated against how the CLI actually waits: the one-shot client takes the first of graphChanged/errorOccurred (graphcode-cli/Sources/main.swift:211), so re-announcing child refusals before the parent broadcast is exactly the ordering needed for a refusal to be seen instead of timed out against.

Non-regression audit

  • Top-level targets short-circuit (subGraphOwner returns nil) — existing paths unchanged; .subGraphCommand/.createNode hit default and are never re-routed.
  • Every callback the routed handlers depend on is forwarded to child stores (deliver, terminate, memory, refine/rollback, predicates) — checked deleteNode, stopNode, updateNode, memoNode, refineNode, messageNode individually.
  • onAnnounceError defaults to nil and only runInSubGraph/tests wire it — no existing construction site changes behavior.
  • Routing scan cost is bounded and only paid when the target is not top-level.

Minor findings (non-blocking, follow-up material)

  1. node pilot / node arm (pilotComposite/armComposite) are not routed — a composite nested inside another composite still can't be piloted from the CLI. Pre-existing and outside the reported command list, but the same class as item 15.
  2. remoteSessionPermitsResolution depends on onReadPresence, deliberately not forwarded to child stores — a routed check-approval on a remote project would skip the confirm-absent probe. Unreachable from the CLI (no check-approve verb), IPC-only, low impact.
  3. onHeartbeatEnabled not forwarded — a routed node update setting a heartbeat interval on a child would be refused even with the experiment on. Practically unreachable since heartbeat children can't be created inside composites (same gate, same nil).
  4. Nit: SubGraphErrorSink.drained returns without clearing — the name promises a drain; harmless today since the sink is discarded immediately after.

None of these block: the reported bug is fixed, refusal messages are preserved, and the failure modes that changed are strict improvements (silent no-ops becoming announced errors).

…dges

A per-command sub-graph store cannot hold a timer: one armed there died
with the store, so a goal loop inside a composite never had its stop
condition polled, and node update's --poll or a promotion re-armed a
poller into a store that was deallocated the moment the command
finished — silently inert. Sub-graph stores now hand poller and
heartbeat arm/cancel requests up through an ordered sink; the project
store applies them (gated on the owning composite having been piloted,
so a template's goal can never resolve work that never ran), and its
ticks descend into the sub-graph, write the result back, and roll the
composite up. Predicate fingerprints and failure tails move into a
cache shared across those one-shot evaluations, so a failing predicate
is still relayed once, not every poll. Piloting, spawning, and boot
restoration arm the recurrence the pilot makes real, and deleting a
composite cancels its workers' timers.

An edge whose endpoints do not share a sub-graph is now refused out
loud instead of dropped silently: no edge may span two graphs.
@scgopi

scgopi commented Aug 30, 2026

Copy link
Copy Markdown
Owner Author

Both review findings are fixed in 61829e1, plus the nits.

1. Ephemeral poller — fixed via the "forward the re-arm up" shape.
Sub-graph stores are per-command and now hold no timers at all: armGoalPoller/cancelGoalPoller/armHeartbeat/cancelHeartbeat at subGraphDepth > 0 hand the request up through an ordered RecurrenceSink (drained by the parent after child.handle, so an update's cancel-then-rearm lands as a pair). The project store owns recurrence for the whole tree:

  • Arms are gated on the direct owning composite being piloted or armed — an unpiloted template's goal can never be polled into resolving work that never ran (covered by anUnpilotedChildsGoalIsNeverPolled).
  • Ticks descend: evaluateGoalDescending/deliverHeartbeatDescending find the owning sub-graph, run the evaluation in a one-shot child store, write the child graph back, roll the composite up, and broadcast.
  • Predicate fingerprints + failure tails moved into a GoalEvaluationCache box shared between the project store and its one-shot evaluation children — without that, each one-shot tick would re-relay the same failure to the session every poll.
  • pilotComposite, composite spawnInstance, and ensureUnattendedSessions (boot) now arm the recurrence piloting makes real; removeSingleNode cancels sub-graph workers' timers on delete. This also fixes the pre-existing leak noted on deinit (per-command stores no longer hold sleeping tasks).

2. Silent edge drop — fixed. createEdge now refuses out loud: "edge refused: an edge may not span two graphs — …" when an endpoint lives inside a composite (routable-pair case was already routed), "edge refused: no loop in this graph" when it exists nowhere. Duplicate-of-same-kind still collapses quietly, as documented.

Nits. SubGraphErrorSink.drained now actually drains; new routed tests for node update, node stop, and node promote on a child, plus two edge-refusal tests and three recurrence tests (pilot arming, update re-arm reaching the project store, unpiloted gate).

Test run with 1309 tests in 141 suites passed (was 1301); swiftlint 0 errors; swift-format clean on touched files.

…essing

# Conflicts:
#	GraphcodeKit/Sources/GraphStore.swift
@scgopi
scgopi merged commit 8aec47a into main Aug 30, 2026
1 check passed
@scgopi
scgopi deleted the fix/217-subgraph-addressing branch August 31, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant