diff --git a/CHANGELOG.md b/CHANGELOG.md index d2aa8b3..fdf5e18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ called out explicitly under **Changed** or **Removed**. nested more than three deep, or one over the same array as a loop around it, as Kizen's builder does. `kizen docs show automation` has a worked example. +- **`automations steps add` of a `loop` adds its `next_or_finish` and its + item variable, as Kizen's builder does.** `steps remove` of a loop with an + empty body removes its closer with it and keeps the steps after it. ### Fixed diff --git a/scripts/cli-tree-baseline.txt b/scripts/cli-tree-baseline.txt index 4ce0d80..2369bbe 100644 --- a/scripts/cli-tree-baseline.txt +++ b/scripts/cli-tree-baseline.txt @@ -1070,17 +1070,22 @@ │ add Insert one step. The spec is a single AutomationDef step (step_type + │ │ config block; field_refs welcome); placement comes from the flags, and │ │ by default the parent's existing children move under the new step, except │ -│ a next_or_finish or merge_branches, which stays on its loop or group. │ +│ a next_or_finish or merge_branches, which stays on its loop or group. A │ +│ loop comes with its next_or_finish and a new `_item` variable, and │ +│ the parent's children follow the next_or_finish. │ │ remove Splice one step out of the graph (children adopt its parent, go_to │ -│ references retarget). A condition, goal or loop with children needs │ -│ --cascade, and a next_or_finish goes only with its loop. │ +│ references retarget). A condition or goal with children, or a loop with a │ +│ body, needs --cascade. A loop with an empty body goes with its │ +│ next_or_finish, which goes only with its loop. │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ Usage: automations steps add [OPTIONS] {api_name} Insert one step. The spec is a single AutomationDef step (step_type + config block; field_refs welcome); placement comes from the flags, and by default the parent's existing children move under - the new step, except a next_or_finish or merge_branches, which stays on its loop or group. + the new step, except a next_or_finish or merge_branches, which stays on its loop or group. A loop + comes with its next_or_finish and a new `_item` variable, and the parent's children follow + the next_or_finish. ╭─ Arguments ──────────────────────────────────────────────────────────────────────────────────────╮ │ * api_name Automation api_name. [required] │ @@ -1141,8 +1146,8 @@ Usage: automations steps remove [OPTIONS] {api_name} {key} Splice one step out of the graph (children adopt its parent, go_to references retarget). A - condition, goal or loop with children needs --cascade, and a next_or_finish goes only with its - loop. + condition or goal with children, or a loop with a body, needs --cascade. A loop with an empty body + goes with its next_or_finish, which goes only with its loop. ╭─ Arguments ──────────────────────────────────────────────────────────────────────────────────────╮ │ * api_name Automation api_name. [required] │ diff --git a/src/kizen_builder/cli/steps.py b/src/kizen_builder/cli/steps.py index 00ead77..221ca32 100644 --- a/src/kizen_builder/cli/steps.py +++ b/src/kizen_builder/cli/steps.py @@ -243,7 +243,9 @@ def steps_add( """Insert one step. The spec is a single AutomationDef step (step_type + config block; field_refs welcome); placement comes from the flags, and by default the parent's existing children move under the new step, except - a next_or_finish or merge_branches, which stays on its loop or group.""" + a next_or_finish or merge_branches, which stays on its loop or group. A + loop comes with its next_or_finish and a new `_item` variable, and + the parent's children follow the next_or_finish.""" if bool(parent) == root: err_console.print("[red]error:[/red] pass exactly one of --parent or --root") raise typer.Exit(code=2) @@ -251,14 +253,16 @@ def steps_add( def mutate(payload: dict[str, Any], raw: dict[str, Any]) -> Any: target_object = (raw.get("custom_object") or {}).get("name") + parent_key, adopt = parent or None, not leaf + if spec.get("step_type") == "loop": + loop, closer, item = auto_tools.build_wire_loop( + spec, payload, target_object + ) + return step_tools.insert_loop( + payload, loop, closer, item, parent_key, branch, adopt + ) wire = auto_tools.build_wire_step(spec, payload, target_object) - return step_tools.insert_step( - payload, - wire, - parent_key=parent or None, - branch=branch, - adopt_children=not leaf, - ) + return step_tools.insert_step(payload, wire, parent_key, branch, adopt) _run_step_patch(api_name, mutate, dry_run, yes, json_out, from_stdin) @@ -275,8 +279,9 @@ def steps_remove( json_out: bool = typer.Option(False, "--json", help="Emit JSON."), ) -> None: """Splice one step out of the graph (children adopt its parent, go_to - references retarget). A condition, goal or loop with children needs - --cascade, and a next_or_finish goes only with its loop.""" + references retarget). A condition or goal with children, or a loop with a + body, needs --cascade. A loop with an empty body goes with its + next_or_finish, which goes only with its loop.""" def mutate(payload: dict[str, Any], raw: dict[str, Any]) -> Any: return step_tools.remove_step(payload, key, cascade=cascade) diff --git a/src/kizen_builder/docs/specs/automation-step.md b/src/kizen_builder/docs/specs/automation-step.md index 2cf2c92..21c9783 100644 --- a/src/kizen_builder/docs/specs/automation-step.md +++ b/src/kizen_builder/docs/specs/automation-step.md @@ -99,16 +99,28 @@ Consequences worth knowing: `next_or_finish` or `merge_branches` child is never adopted, so `--parent ` puts the new step at the head of the loop body, and `--parent ` puts it right after the loop. An `exit_loop` adds like any - step. A `loop` or `next_or_finish` can't be added on its own: add the loop, - its closer and its item variable with `kizen automations update - --spec-file`. + step. +- **`add` of a `loop` adds its `next_or_finish` and its item variable too**, as + Kizen's builder does. The spec is `{"step_type": "loop", "step_loop": + {"array_variable": ""}}`, and the array must already be declared by + an `initialize_variable` step; the CLI never creates it. The item variable is + always created, named `_item` (`_item_2`, `_3`… when the name + is taken), with the array's type, in a new `initialize_variable` step after + the other variable steps. A spec that sets `step_loop.item_variable` is + refused. The loop's body starts empty, and the parent's existing children + follow the `next_or_finish`, so they run once, after the loop (`--leaf` + leaves them on the parent). A `next_or_finish` can't be added on its own. + Confirmed live 2026-10-27, with `roundtrip --execute` clean after each add + and remove. - **`remove` splices:** children adopt the removed step's parent and inherit its branch position, and `go_to` references retarget to that parent. Removing a condition or goal that has children requires `--cascade`, which deletes the - whole subtree. So does a loop with steps under it, where the subtree is the - body and everything after the loop. A `next_or_finish` can't be removed, even - with `--cascade`: remove its loop instead. Removing the last body step leaves - an empty body, which is valid. + whole subtree. So does a loop with body steps, where the subtree is the body + and everything after the loop. A loop with an empty body is removed with its + `next_or_finish`, the steps after it move up, and its item variable step + stays. A `next_or_finish` can't be removed, even with `--cascade`: remove its + loop instead. Removing the last body step leaves an empty body, which is + valid. - **The post-apply diff is positional**, so a mid-graph change shows up as every later step shifting. Verify structure with `show` and treat the diff as the audit trail, not as a minimal changeset. diff --git a/src/kizen_builder/docs/specs/automation.md b/src/kizen_builder/docs/specs/automation.md index eacd025..2d7fa5b 100644 --- a/src/kizen_builder/docs/specs/automation.md +++ b/src/kizen_builder/docs/specs/automation.md @@ -138,7 +138,9 @@ sets a step's own severity. Omitted, the step inherits. Confirmed live A loop runs its body once per item of an array variable. Loops are branch group initiators, closed by `next_or_finish` instead of `merge_branches`. A -spec names all six parts; the CLI adds none of them: +spec names all six parts; `create` and `update` add none of them. `steps add` +of a loop adds its closer and item variable (see `kizen docs show +automation-step`): 1. **The array variable:** an `initialize_variable` step with `is_array: true`. 2. **Its item variable:** an `initialize_variable` step with `sources: []`, diff --git a/src/kizen_builder/tools/automations.py b/src/kizen_builder/tools/automations.py index c65cc5f..38235d7 100644 --- a/src/kizen_builder/tools/automations.py +++ b/src/kizen_builder/tools/automations.py @@ -424,25 +424,97 @@ def build_wire_step( Linkage fields are ignored here — placement belongs to insert_step. Any ``id`` on the spec is dropped too: this builds a *new* step, and a spec authored by copying an existing step's ``steps get`` output must not - carry that step's id along and hijack its identity. + carry that step's id along and hijack its identity. A ``loop`` goes + through :func:`build_wire_loop` instead. """ + from kizen_builder.tools.plans import PlanError + + if spec.get("step_type") == "next_or_finish": + raise PlanError( + "a next_or_finish can't be added on its own; 'steps add' of a loop " + "adds the loop's next_or_finish with it." + ) + return _build_wire(spec, payload, target_object) + + +def build_wire_loop( + spec: dict[str, Any], payload: dict[str, Any], target_object: str | None +) -> tuple[dict[str, Any], dict[str, Any], dict[str, Any] | None]: + """Build a loop, its ``next_or_finish`` and its item variable step, as + Kizen's builder does. The item variable is always created, named + ``_item`` (``_2``, ``_3``… when taken), with the array's type. It is + ``None`` when the payload declares no such array: validation then refuses + the loop. Placement belongs to :func:`steps.insert_loop`. + """ + from kizen_builder.tools.planners.automations import _unwrap_variable_name + from kizen_builder.tools.plans import PlanError + + block = dict(spec.get("step_loop") or {}) + array = _unwrap_variable_name(block.get("array_variable")) + if "item_variable" in block: + raise PlanError( + "step_loop.item_variable can't be set in 'steps add': the item " + f"variable is created automatically, named '{array}_item'. Remove it " + "from the spec." + ) + declared: dict[str, dict[str, Any]] = {} + for s in payload["steps"]: + var = (s.get("action_initialize_variable") or {}).get("variable") + if s["type"] == "initialize_variable" and isinstance(var, dict): + declared.setdefault(var.get("name") or "", var) + item_name, n = f"{array}_item", 2 + while item_name in declared: + item_name, n = f"{array}_item_{n}", n + 1 + + block["item_variable"] = item_name + loop = _build_wire( + { + **spec, + "step_loop": block, + "description": spec.get("description") or f"For each item in '{array}'", + }, + payload, + target_object, + ) + closer = _build_wire( + { + "step_type": "next_or_finish", + "description": "Next item or finish loop and continue", + }, + payload, + target_object, + ) + item = None + if array in declared: + variable = { + "name": item_name, + "data_type": declared[array].get("data_type"), + "is_array": False, + } + if declared[array].get("data_subtype"): + variable["data_subtype"] = declared[array]["data_subtype"] + item = _build_wire( + { + "step_type": "initialize_variable", + "description": "Item Reference Variable", + "action_initialize_variable": {"sources": [], "variable": variable}, + }, + payload, + target_object, + ) + return loop, closer, item + + +def _build_wire( + spec: dict[str, Any], payload: dict[str, Any], target_object: str | None +) -> dict[str, Any]: from kizen_builder.models.spec import AutomationStepDef from kizen_builder.tools.planners.automations import ( LiveContext, _build_step_payload, ) - from kizen_builder.tools.plans import PlanError from kizen_builder.tools.steps import synthesize_key - counterpart = {"loop": "next_or_finish", "next_or_finish": "loop"}.get( - spec.get("step_type", "") - ) - if counterpart: - raise PlanError( - f"a {spec['step_type']} is added together with its {counterpart} and " - "the loop's item variable, which one 'steps add' can't do. Add it " - "with 'kizen automations update --spec-file'." - ) spec = dict(spec) for linkage in ("parent_key", "parent_branch", "id"): spec.pop(linkage, None) diff --git a/src/kizen_builder/tools/steps.py b/src/kizen_builder/tools/steps.py index 1bba449..0bd3c5f 100644 --- a/src/kizen_builder/tools/steps.py +++ b/src/kizen_builder/tools/steps.py @@ -88,7 +88,9 @@ def remove_step( Without cascade: children adopt the removed step's parent (inheriting its branch position), and go_to steps targeting it are retargeted to the same parent. Refuses to splice a condition/goal that still has children - — their yes/no positions have no meaning under a different parent. + — their yes/no positions have no meaning under a different parent — or a + loop with a body. A loop with an empty body goes with its + next_or_finish, whose children take the loop's place. Returns a report: removed keys, (child, new_parent) re-parentings, and retargeted go_to keys. @@ -111,15 +113,19 @@ def remove_step( "orphan its yes/no branches. Remove or re-parent them first, " "or pass cascade=True to delete the whole subtree." ) - if dead["type"] == "loop" and kids: - raise PlanError( - f"'{key}' is a loop with steps under it " - f"({', '.join(k['key'] for k in kids)}); removing it alone would " - "leave its next_or_finish without a loop. Remove its body first, " - "or pass cascade=True to delete the loop with its body and " - "everything after it." - ) doomed = {key} + if dead["type"] == "loop": + body = [k["key"] for k in kids if k["type"] != "next_or_finish"] + if body: + raise PlanError( + f"'{key}' is a loop with steps under it ({', '.join(body)}); " + "removing it alone would leave its next_or_finish without a " + "loop. Remove its body first, or pass cascade=True to delete " + "the loop with its body and everything after it." + ) + closers = [k["key"] for k in kids] + doomed.update(closers) + kids = [c for k in closers for c in children_of(payload, k)] new_parent = dead.get("parent_key") gotos = _gotos_pointing_at(payload, doomed) @@ -209,6 +215,43 @@ def insert_step( return report +def insert_loop( + payload: dict[str, Any], + loop: dict[str, Any], + closer: dict[str, Any], + item: dict[str, Any] | None, + parent_key: str | None = None, + branch: str = "", + adopt_children: bool = True, +) -> dict[str, Any]: + """Link a built loop, its ``next_or_finish`` and its item variable step. + + The loop goes where :func:`insert_step` puts any step, with an empty body. + With ``adopt_children`` the parent's children follow the closer, so they + run once after the loop, as in Kizen's builder. The item variable step goes + after the last ``initialize_variable``, since variables come first. + """ + report = insert_step(payload, loop, parent_key, branch, adopt_children) + insert_step(payload, closer, loop["key"]) + out: dict[str, Any] = { + "key": loop["key"], + "closer": closer["key"], + "item_variable": None, + "adopted": report["adopted"], + } + if item is not None: + last_var = max( + (s for s in payload["steps"] if s["type"] == "initialize_variable"), + key=lambda s: s.get("order") or 0, + ) + insert_step(payload, item, last_var["key"]) + out["item_variable"] = { + "name": item["action_initialize_variable"]["variable"]["name"], + "key": item["key"], + } + return out + + def edit_step( payload: dict[str, Any], key: str, patch: dict[str, Any] ) -> dict[str, Any]: diff --git a/tests/test_steps.py b/tests/test_steps.py index 713a4aa..a25aaab 100644 --- a/tests/test_steps.py +++ b/tests/test_steps.py @@ -256,9 +256,8 @@ def test_remove_loop_with_steps_under_it_refuses(loops: dict) -> None: with pytest.raises(PlanError) as e: remove_step(loops, "s10_loop") assert str(e.value) == ( - "'s10_loop' is a loop with steps under it (s11_delay, " - "s12_next_or_finish); removing it alone would leave its next_or_finish " - "without a loop. Remove its body first, or pass cascade=True to delete " + "'s10_loop' is a loop with steps under it (s11_delay); removing it " + "alone would leave its next_or_finish without a loop. Remove its body first, or pass cascade=True to delete " "the loop with its body and everything after it." ) @@ -300,22 +299,223 @@ def test_steps_add_builds_an_exit_loop(loops: dict) -> None: assert validate_payload(loops) == [] -@pytest.mark.parametrize( - "step_type, counterpart", - [("loop", "next_or_finish"), ("next_or_finish", "loop")], -) -def test_steps_add_refuses_half_a_loop( - loops: dict, step_type: str, counterpart: str -) -> None: +# `steps add` of a loop: the loop, its next_or_finish and its item variable. + + +def _loop_spec(array: str) -> dict: + return {"step_type": "loop", "step_loop": {"array_variable": array}} + + +def _chain() -> dict: + """arr (String array of three), seen (String), then stops a → b.""" + from kizen_builder.tools.automations import build_wire_step + + payload: dict = {"name": "Chain", "api_name": "chain", "type": "global"} + payload.update(steps=[], triggers=[]) + specs = [ + { + "key": "arr", + "step_type": "initialize_variable", + "action_initialize_variable": { + "variable": {"name": "arr", "data_type": "string", "is_array": True}, + "sources": [ + { + "source_type": "static", + "source_subtype": "string", + "value": ["x", "y", "z"], + } + ], + "array_aggregation_mode": "add_all", + }, + }, + { + "key": "seen", + "step_type": "initialize_variable", + "action_initialize_variable": { + "variable": {"name": "seen", "data_type": "string"}, + "sources": [], + }, + }, + {"key": "a", "step_type": "stop_execution", "action_stop_execution": {}}, + {"key": "b", "step_type": "stop_execution", "action_stop_execution": {}}, + ] + parent = None + for spec in specs: + insert_step(payload, build_wire_step(spec, payload, None), parent) + parent = spec["key"] + return payload + + +def _add_loop(payload: dict, array: str, parent: str, **placement) -> dict: + from kizen_builder.tools.automations import build_wire_loop + from kizen_builder.tools.steps import insert_loop + + target = "object_with_workflow" if payload["type"] == "record_based" else None + loop, closer, item = build_wire_loop(_loop_spec(array), payload, target) + return insert_loop(payload, loop, closer, item, parent, **placement) + + +def _chain_keys(payload: dict) -> list[str]: + return [s["key"] for s in sorted(payload["steps"], key=lambda s: s["order"])] + + +def _variables_first(payload: dict) -> bool: + types = [s["type"] for s in sorted(payload["steps"], key=lambda s: s["order"])] + n = types.count("initialize_variable") + return set(types[:n]) == {"initialize_variable"} + + +def test_steps_add_loop_mid_chain_follows_with_the_old_children() -> None: + payload = _chain() + report = _add_loop(payload, "arr", "a") + loop, closer = report["key"], report["closer"] + item = report["item_variable"] + assert item["name"] == "arr_item" + assert report["adopted"] == ["b"] + assert _chain_keys(payload) == ["arr", "seen", item["key"], "a", loop, closer, "b"] + assert find_step(payload, "b")["parent_key"] == closer + assert find_step(payload, closer)["parent_key"] == loop + assert find_step(payload, loop)["step_loop"]["item_variable"] == "arr_item" + assert find_step(payload, loop)["description"] == "For each item in 'arr'" + assert find_step(payload, closer)["description"] == ( + "Next item or finish loop and continue" + ) + item_step = find_step(payload, item["key"]) + assert item_step["description"] == "Item Reference Variable" + assert item_step["action_initialize_variable"]["sources"] == [] + variable = item_step["action_initialize_variable"]["variable"] + assert (variable["data_type"], variable["is_array"]) == ("string", False) + assert "data_subtype" not in variable + assert _variables_first(payload) + assert validate_payload(payload) == [] + + +def test_steps_add_loop_under_the_last_variable_step() -> None: + payload = _chain() + report = _add_loop(payload, "arr", "seen") + item, loop = report["item_variable"]["key"], report["key"] + assert report["adopted"] == ["a"] + assert _chain_keys(payload) == [ + "arr", + "seen", + item, + loop, + report["closer"], + "a", + "b", + ] + assert find_step(payload, loop)["parent_key"] == item + assert find_step(payload, item)["parent_key"] == "seen" + assert validate_payload(payload) == [] + + +def test_steps_add_loop_as_a_leaf() -> None: + payload = _chain() + report = _add_loop(payload, "arr", "b", adopt_children=False) + assert report["adopted"] == [] + assert find_step(payload, report["key"])["parent_key"] == "b" + assert children_of(payload, report["closer"]) == [] + assert validate_payload(payload) == [] + + +def test_steps_add_loop_in_a_condition_branch(loops: dict) -> None: + report = _add_loop(loops, "arr1", "s05_condition", branch="yes") + loop = find_step(loops, report["key"]) + assert (loop["parent_key"], loop["parent_yes_no"]) == ("s05_condition", "yes") + assert report["adopted"] == ["s06_delay"] + moved = find_step(loops, "s06_delay") + assert (moved["parent_key"], moved["parent_yes_no"]) == (report["closer"], "") + assert report["item_variable"]["name"] == "arr1_item_2" + assert _variables_first(loops) + assert validate_payload(loops) == [] + + +def test_steps_add_loop_over_records_copies_the_subtype() -> None: + payload = live_to_payload(load_fixture("automations/new_features_loops.raw.json")) + report = _add_loop(payload, "array_var_obj", "s08_file_content_extraction") + assert report["item_variable"]["name"] == "array_var_obj_item_2" + variable = find_step(payload, report["item_variable"]["key"])[ + "action_initialize_variable" + ]["variable"] + assert variable["data_type"] == "entity" + assert variable["data_subtype"] == "8c3f0149-6915-4370-940f-2e61848d6842" + assert find_step(payload, "s09_loop")["parent_key"] == report["closer"] + assert _variables_first(payload) + assert validate_payload(payload) == [] + + +def test_steps_add_loop_refuses_an_item_variable(loops: dict) -> None: + from kizen_builder.tools.automations import build_wire_loop + + before = [dict(s) for s in loops["steps"]] + spec = _loop_spec("arr0") + spec["step_loop"]["item_variable"] = "mine" + with pytest.raises(PlanError) as e: + build_wire_loop(spec, loops, None) + assert str(e.value) == ( + "step_loop.item_variable can't be set in 'steps add': the item variable " + "is created automatically, named 'arr0_item'. Remove it from the spec." + ) + assert loops["steps"] == before + + +def test_steps_add_loop_takes_the_next_free_item_name() -> None: + payload = _chain() + first = _add_loop(payload, "arr", "a") + second = _add_loop(payload, "arr", first["closer"]) + assert first["item_variable"]["name"] == "arr_item" + assert second["item_variable"]["name"] == "arr_item_2" + assert find_step(payload, "b")["parent_key"] == second["closer"] + assert validate_payload(payload) == [] + + +def test_steps_add_loop_over_an_undeclared_array_creates_no_item() -> None: + from kizen_builder.tools.automations import build_wire_loop + + payload = _chain() + *_, item = build_wire_loop(_loop_spec("nope"), payload, None) + assert item is None + report = _add_loop(payload, "nope", "a") + assert report["item_variable"] is None + assert validate_payload(payload) == [ + f"loop '{report['key']}' iterates 'nope', which is not an array " + "variable of records, strings or UUIDs (server 400s otherwise)" + ] + + +def test_steps_add_loop_over_the_outer_loops_array_is_refused(loops: dict) -> None: + report = _add_loop(loops, "arr0", "s04_loop") + assert any( + f"loop '{report['key']}' iterates 'arr0', which loop 's04_loop' around it" in p + for p in validate_payload(loops) + ) + + +def test_steps_add_refuses_a_lone_next_or_finish(loops: dict) -> None: from kizen_builder.tools.automations import build_wire_step - spec = {"step_type": step_type} - if step_type == "loop": - spec["step_loop"] = {"array_variable": "arr0", "item_variable": "arr0_item"} with pytest.raises(PlanError) as e: - build_wire_step(spec, loops, "object_with_workflow") + build_wire_step({"step_type": "next_or_finish"}, loops, "object_with_workflow") assert str(e.value) == ( - f"a {step_type} is added together with its {counterpart} and the loop's " - "item variable, which one 'steps add' can't do. Add it with " - "'kizen automations update --spec-file'." + "a next_or_finish can't be added on its own; 'steps add' of a loop adds " + "the loop's next_or_finish with it." ) + + +def test_remove_empty_loop_takes_its_closer() -> None: + payload = _chain() + report = _add_loop(payload, "arr", "a") + item = report["item_variable"]["key"] + removed = remove_step(payload, report["key"]) + assert removed["removed"] == sorted([report["key"], report["closer"]]) + assert removed["reparented"] == [("b", "a")] + assert _chain_keys(payload) == ["arr", "seen", item, "a", "b"] + assert validate_payload(payload) == [] + + +def test_remove_empty_loop_in_a_branch_keeps_the_branch(loops: dict) -> None: + report = _add_loop(loops, "arr1", "s05_condition", branch="yes") + remove_step(loops, report["key"]) + moved = find_step(loops, "s06_delay") + assert (moved["parent_key"], moved["parent_yes_no"]) == ("s05_condition", "yes") + assert validate_payload(loops) == []