From 34b3f8fc3cde66dc7c658974dbde23547a3d3c30 Mon Sep 17 00:00:00 2001 From: William Cory Date: Wed, 18 Mar 2026 12:53:03 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix:=20allow=20nested=20Loop/Ral?= =?UTF-8?q?ph=20when=20separated=20by=20structural=20nodes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #111. The nesting guard in both extract.ts and scheduler.ts used a monotonic flag (ralphId / inRalph) that propagated through the entire subtree. This rejected even though only direct nesting () should be disallowed. Changed both guards to check parentIsRalph (immediate parent) instead of any-ancestor. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/dom/extract.ts | 6 +- src/engine/scheduler.ts | 10 +- tests/nested-ralph-bug.test.ts | 165 +++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 7 deletions(-) create mode 100644 tests/nested-ralph-bug.test.ts diff --git a/src/dom/extract.ts b/src/dom/extract.ts index ea4d7cd7..f5c3a907 100644 --- a/src/dom/extract.ts +++ b/src/dom/extract.ts @@ -128,6 +128,7 @@ export function extractFromHost( path: number[]; iteration: number; ralphId?: string; + parentIsRalph: boolean; parallelStack: { id: string; max?: number }[]; /** * Stack of active contexts (outermost -> innermost). @@ -144,7 +145,7 @@ export function extractFromHost( const worktreeStack = ctx.worktreeStack; if (node.tag === "smithers:ralph") { - if (ralphId) { + if (ctx.parentIsRalph) { throw new Error("Nested is not supported."); } const id = resolveStableId(node.rawProps?.id, "ralph", ctx.path); @@ -319,13 +320,14 @@ export function extractFromHost( path: nextPath, iteration, ralphId, + parentIsRalph: node.tag === "smithers:ralph", parallelStack: nextParallelStack, worktreeStack: nextWorktreeStack, }); } } - walk(root, { path: [], iteration: 0, parallelStack: [], worktreeStack: [] }); + walk(root, { path: [], iteration: 0, parentIsRalph: false, parallelStack: [], worktreeStack: [] }); return { xml: toXmlNode(root), tasks, mountedTaskIds }; } diff --git a/src/engine/scheduler.ts b/src/engine/scheduler.ts index 354a1100..28cdcc5a 100644 --- a/src/engine/scheduler.ts +++ b/src/engine/scheduler.ts @@ -64,22 +64,22 @@ export function buildPlanTree(xml: XmlNode | null): { function walk( node: XmlNode, - ctx: { path: number[]; inRalph: boolean }, + ctx: { path: number[]; parentIsRalph: boolean }, ): PlanNode | null { if (node.kind === "text") return null; const tag = node.tag; - if (ctx.inRalph && tag === "smithers:ralph") { + if (ctx.parentIsRalph && tag === "smithers:ralph") { throw new Error("Nested is not supported."); } const children: PlanNode[] = []; let elementIndex = 0; + const isRalph = tag === "smithers:ralph"; for (const child of node.children) { const nextPath = child.kind === "element" ? [...ctx.path, elementIndex++] : ctx.path; - const nextInRalph = ctx.inRalph || tag === "smithers:ralph"; - const built = walk(child, { path: nextPath, inRalph: nextInRalph }); + const built = walk(child, { path: nextPath, parentIsRalph: isRalph }); if (built) children.push(built); } @@ -133,7 +133,7 @@ export function buildPlanTree(xml: XmlNode | null): { return { kind: "group", children }; } - const plan = walk(xml, { path: [], inRalph: false }); + const plan = walk(xml, { path: [], parentIsRalph: false }); return { plan, ralphs }; } diff --git a/tests/nested-ralph-bug.test.ts b/tests/nested-ralph-bug.test.ts new file mode 100644 index 00000000..6526bbc5 --- /dev/null +++ b/tests/nested-ralph-bug.test.ts @@ -0,0 +1,165 @@ +/** + * Reproduction for https://github.com/jjhub-ai/smithers/issues/111 + * + * Nested / separated by a structural node (, , + * etc.) should be allowed. Direct nesting () should remain rejected. + */ +import { describe, expect, test } from "bun:test"; +import { buildPlanTree } from "../src/engine/scheduler"; +import { extractFromHost, type HostElement } from "../src/dom/extract"; +import { el } from "./helpers"; + +// ── Minimal HostElement factory for extract.ts tests ───────────────── +function hostEl( + tag: string, + rawProps: Record = {}, + children: HostElement[] = [], +): HostElement { + const props: Record = {}; + for (const [k, v] of Object.entries(rawProps)) { + if (typeof v === "string") props[k] = v; + } + return { kind: "element", tag, props, rawProps, children }; +} + +// ═══════════════════════════════════════════════════════════════════════ +// 1. scheduler.ts – buildPlanTree +// ═══════════════════════════════════════════════════════════════════════ +describe("issue #111 – buildPlanTree nested ralph", () => { + test("direct nesting is rejected", () => { + const xml = el("smithers:ralph", { id: "outer" }, [ + el("smithers:ralph", { id: "inner" }, [ + el("smithers:task", { id: "t1" }), + ]), + ]); + expect(() => buildPlanTree(xml)).toThrow("Nested "); + }); + + test("ralph > sequence > ralph is allowed", () => { + const xml = el("smithers:ralph", { id: "outer" }, [ + el("smithers:sequence", {}, [ + el("smithers:ralph", { id: "inner" }, [ + el("smithers:task", { id: "t1" }), + ]), + ]), + ]); + const { plan, ralphs } = buildPlanTree(xml); + expect(plan).toBeDefined(); + expect(ralphs).toHaveLength(2); + expect(ralphs.map((r) => r.id).sort()).toEqual(["inner", "outer"]); + }); + + test("ralph > parallel > ralph is allowed", () => { + const xml = el("smithers:ralph", { id: "outer" }, [ + el("smithers:parallel", {}, [ + el("smithers:ralph", { id: "inner" }, [ + el("smithers:task", { id: "t1" }), + ]), + ]), + ]); + const { plan, ralphs } = buildPlanTree(xml); + expect(plan).toBeDefined(); + expect(ralphs).toHaveLength(2); + }); + + test("ralph > worktree (group) > ralph is allowed", () => { + const xml = el("smithers:ralph", { id: "outer" }, [ + el("smithers:worktree", { id: "wt", path: "/tmp/test" }, [ + el("smithers:ralph", { id: "inner" }, [ + el("smithers:task", { id: "t1" }), + ]), + ]), + ]); + const { plan, ralphs } = buildPlanTree(xml); + expect(plan).toBeDefined(); + expect(ralphs).toHaveLength(2); + }); + + test("three levels: ralph > sequence > ralph > sequence > ralph", () => { + const xml = el("smithers:ralph", { id: "L1" }, [ + el("smithers:sequence", {}, [ + el("smithers:ralph", { id: "L2" }, [ + el("smithers:sequence", {}, [ + el("smithers:ralph", { id: "L3" }, [ + el("smithers:task", { id: "t1" }), + ]), + ]), + ]), + ]), + ]); + const { ralphs } = buildPlanTree(xml); + expect(ralphs).toHaveLength(3); + }); + + test("sibling ralphs inside sequence are allowed", () => { + const xml = el("smithers:sequence", {}, [ + el("smithers:ralph", { id: "a" }, [ + el("smithers:task", { id: "t1" }), + ]), + el("smithers:ralph", { id: "b" }, [ + el("smithers:task", { id: "t2" }), + ]), + ]); + const { ralphs } = buildPlanTree(xml); + expect(ralphs).toHaveLength(2); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════ +// 2. extract.ts – extractFromHost +// ═══════════════════════════════════════════════════════════════════════ +describe("issue #111 – extractFromHost nested ralph", () => { + test("direct nesting is rejected", () => { + const root = hostEl("smithers:ralph", { id: "outer" }, [ + hostEl("smithers:ralph", { id: "inner" }, [ + hostEl("smithers:task", { + id: "t1", + output: "out", + __smithersPayload: "data", + }), + ]), + ]); + expect(() => extractFromHost(root)).toThrow("Nested "); + }); + + test("ralph > sequence > ralph is allowed", () => { + const root = hostEl("smithers:ralph", { id: "outer" }, [ + hostEl("smithers:sequence", {}, [ + hostEl("smithers:ralph", { id: "inner" }, [ + hostEl("smithers:task", { + id: "t1", + output: "out", + __smithersPayload: "data", + }), + ]), + ]), + ]); + const result = extractFromHost(root); + expect(result.tasks).toHaveLength(1); + expect(result.tasks[0].ralphId).toBe("inner"); + }); + + test("inner task gets innermost ralphId", () => { + const root = hostEl("smithers:ralph", { id: "outer" }, [ + hostEl("smithers:sequence", {}, [ + hostEl("smithers:ralph", { id: "inner" }, [ + hostEl("smithers:task", { + id: "t1", + output: "out", + __smithersPayload: "data", + }), + ]), + hostEl("smithers:task", { + id: "t2", + output: "out", + __smithersPayload: "data", + }), + ]), + ]); + const result = extractFromHost(root); + const t1 = result.tasks.find((t) => t.nodeId === "t1")!; + const t2 = result.tasks.find((t) => t.nodeId === "t2")!; + expect(t1.ralphId).toBe("inner"); + expect(t2.ralphId).toBe("outer"); + }); +});