From fe7ce60400fa73a8f77ff009b95fe0f12ac4946c Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:16:03 -0500 Subject: [PATCH 01/38] docs: list-driven steps proposal revised after review, with the stage 1 plan Co-Authored-By: Claude Fable 5.1 --- docs/proposals/list-driven-steps.md | 184 +- .../plans/2026-09-11-list-driven-steps.md | 1632 +++++++++++++++++ 2 files changed, 1763 insertions(+), 53 deletions(-) create mode 100644 docs/superpowers/plans/2026-09-11-list-driven-steps.md diff --git a/docs/proposals/list-driven-steps.md b/docs/proposals/list-driven-steps.md index 4f9101b2..bf8bf8eb 100644 --- a/docs/proposals/list-driven-steps.md +++ b/docs/proposals/list-driven-steps.md @@ -1,7 +1,10 @@ # Proposal: list-driven steps (`for_each`) -Status: **needs approval**. Written for MCP feedback ticket T003; nothing here -is implemented. +Status: **needs approval**, revised 2026-09-11 after review. Written for MCP +feedback ticket T003; nothing here is implemented. The review found that the +first draft could not express either target template - `music-video` pairs a +slice step with a shot step per entry, and `dialogue-short`'s per-shot +reference lists vary in length - and the shape below is amended for both. ## The ask, as filed @@ -73,6 +76,25 @@ and downstream step per entry, in list order. - `item:` inside the step is that entry: `item:prompt` is one field, bare `item:` is the whole entry (a list of plain strings is the common case). + A field holds whatever the entry wrote there - a string, a number, a list + or an object - and the pass splices it in as definition text. This is not + optional: `dialogue-short`'s shots reference both characters in shots 1 + and 5 and one character in shots 2-4, so a shot's `references` list has to + come from the entry whole (`"references": "item:references"`), and a + scalar-only `item:` could not write that template. An entry may therefore + carry `from_previous_result`, `asset:` and `prompt:` strings; they become + ordinary references in the expanded step and are checked as such (below). +- **Same-key siblings.** Inside a `for_each` over a list, a + `previous_result:`/`from_previous_result` naming *another* `for_each` step + over the same list resolves to the member with the same key: + `"from_previous_result": "slice"` inside `shot` becomes + `slice@wide_open` inside `shot@wide_open`. This is the one pairing the + engine needs and the reason it is not a zip (below): `music-video` slices + the audio bed in one task step and generates the shot in a pipeline step, + so shot *i* reads slice *i* across two steps, and "put both in one entry" + cannot express it. The two groups must name the same list (the same + `variable:` or an identical literal); a same-key reference into a group + over a different list is an error. - `gather:shot` is the list of every member's artifacts, in order, as one value. It is a *list-valued* reference, which is the distinction `previous_result:` cannot make. @@ -83,11 +105,12 @@ Run the expansion as a pass over the definition immediately after `replace_variables` in `Workflow.run` (`dw/workflow.py`), before the run id is computed and before the step loop starts. The pass: -1. replaces each `for_each` step with N ordinary steps, named `shot[wide_open]` +1. replaces each `for_each` step with N ordinary steps, named `shot@wide_open` … (below); -2. substitutes `item:` references inside each copy with that entry's values; +2. substitutes `item:` references inside each copy with that entry's values, + and rewrites a same-key sibling reference to the member with this key; 3. rewrites every `gather:shot` into the explicit list of - `["previous_result:shot[wide_open]", "previous_result:shot[closeup]"]` - + `["previous_result:shot@wide_open", "previous_result:shot@closeup"]` - exactly what a hand-written template contains today. After the pass the definition is an ordinary workflow. Nothing below it @@ -110,14 +133,23 @@ Consequences worth stating: pipeline *loads*, excluding arguments and seed, so N identical expanded pipeline blocks hit one loaded model within the run. The expansion needs no `pipeline_reference` rewriting to get the reuse the hand-written template - gets by hand. `release_pipeline` on a fanned step is the exception: it would - drop the model after the first member and reload it for the second. Refuse - it on a `for_each` step, or honour it only on the last member. + gets by hand (the templates' `pipeline_reference` shots become full + pipeline blocks in the expansion, and `pipeline_reference` inside a + `for_each` step is meaningless). `release_pipeline` on a fanned step would + drop the model after the first member and reload it for the second: the + pass carries it onto the **last member only**, and `release_models` the + same way. +- `realize_args(variables)` runs on the variables *before* substitution + (`dw/arguments.py`, the `isinstance(v, list)` branch hands a list to + `resolve_path_references`). Stage 1 must confirm that an `asset:` inside an + entry object comes out as a path and not as loaded media, or `item:` would + splice a PIL object into the definition. If it loads, exempt the list a + `for_each` names and let the expanded step realize it as any step does. ### Expanded names: the entry's own name before its index -`shot[wide_open]` when the entry is an object carrying a `name`, else -`shot[0]`, `shot[1]`. +`shot@wide_open` when the entry is an object carrying a `name`, else +`shot@0`, `shot@1`. The index alone is tempting and wrong for the cache. Entries are keyed `(workflow id, step name)` in `dw/step_cache.py`, so inserting a shot in the @@ -128,23 +160,46 @@ episode is most of an hour of GPU to add one shot in the middle. A name carried by the entry survives insertion, and it also makes the manifest, the event stream and the gallery read in shot names rather than in ordinals. -Brackets rather than dots or underscores: `.` is already the property -separator in a reference (`previous_result:segment.mask`) and `_` collides -with a hand-written `shot_1`. A step name is otherwise an unrestricted string, -so the bracket form has to be reserved - a hand-written step named `shot[0]` -becomes an error. +`@` rather than dots, underscores or brackets: `.` is already the property +separator in a reference (`previous_result:segment.mask`), `_` and `-` +collide with a hand-written `shot_1`, and `:` is the reference-prefix +separator. Brackets were the first draft and are wrong for a reason that +only shows up later: the name lands in a filename +(`{workflow_id}-{step_name}.{i}`, `dw/workflow.py`), and `[...]` is a shell +glob class - `ls *shot[wide_open]*` matches one character - as well as a +character every gallery and MCP URL would have to encode. `@` is none of +those. A step name is otherwise an unrestricted string, so `@` has to be +reserved - a hand-written step named `shot@0` becomes an error. + +**Entry names are validated.** The name reaches the filesystem and the +step-cache key, so it must match the variable-name pattern +(`validate_variable_name`, `^[a-zA-Z_][a-zA-Z0-9_-]*$`) and be unique within +its list - two entries named `closeup` would otherwise expand to two steps +with one name, one cache entry and one clobbered output file. Both are +errors from the pass, reported with the entry's position. ### Validation The static reference check shipped for T005 (`previous_result_reference_errors`) runs on the unexpanded definition, where -`shot[wide_open]` does not exist yet. `validate_workflow` should run the same -expansion first, using the variables' declared defaults, and check the -expanded steps - which also means an empty or absent default list validates -as zero steps and a `gather:` naming nothing. Order becomes: schema -> -substitute -> expand -> reference check. Schema validation stays where it is, -against the template step, and the schema gains `for_each` on a step plus the -`item:`/`gather:` prefixes wherever a reference is allowed. +`shot@wide_open` does not exist yet. `validate_workflow` should run the same +expansion first and check the expanded steps. It expands **the list the run +will use**: `POST /api/validate` already takes the caller's `arguments` +(`argument_errors` folds them the way `set_variables` does), so the +pre-flight expands the folded list, not the declared default - otherwise it +checks a step set the caller is not about to run. Without arguments the +default is the list, and an empty or absent one validates as zero steps and +a `gather:` naming nothing. Order becomes: schema -> substitute -> expand -> +reference check. Schema validation stays where it is, against the template +step, and the schema gains `for_each` on a step plus the `item:`/`gather:` +prefixes wherever a reference is allowed. + +Two errors deserve their own message rather than the generic "names no +earlier step": `previous_result:shot` where `shot` is a `for_each` group +("use `gather:shot`, or a same-key reference from inside a group over the +same list"), and `gather:` naming a step that is not a group. References an +entry carries (`from_previous_result` inside `item:references`) are checked +in the expanded step like any other, which is where they exist. ### The realized workflow and the manifest @@ -152,21 +207,33 @@ against the template step, and the schema gains `for_each` on a step plus the realized list-driven workflow carries the actual `shots` list and reproduces the same expansion deterministically. **Keep `for_each` in the realized file** rather than writing the expanded steps: it reproduces the run exactly and stays -legible. The manifest names the expanded steps (`shot[wide_open]`), because the +legible. The manifest names the expanded steps (`shot@wide_open`), because the manifest records what ran. That the two name steps differently is deliberate and should be documented where the manifest is. ### Limits and cost -Each entry is a full generation, so the expansion needs a ceiling of its own - -something like 64 steps, refused outright above it, well below -`MAX_ITERATIONS = 10000`, which guards a different thing. The larger -consequence is that a list-driven template's cost is set by the caller: -`list_workflows` quotes a per-workflow figure today, and a six-entry `shots` -list is six times the shot cost. The catalog entry should carry the per-entry -cost and the variable it multiplies by, and the MCP guidance -(`docs/WORKFLOW_GUIDE.md`, "Authoring a workflow from an agent") should say to -quote `cost x len(list)` before running. +Each entry is a full generation, so the expansion needs a ceiling of its own, +well below `MAX_ITERATIONS = 10000`, which guards a different thing. The +ceiling and the step cache's bound have to be stated together: the cache +holds `DEFAULT_MAX_ENTRIES = 50` (`dw/step_cache.py`, LRU), and a run whose +expanded steps exceed it evicts its own earlier members - the insertion case +the naming scheme exists for stops hitting, silently. `music-video` expands +to shots + slices + five fixed steps, so the cache is full at about twenty +shots. Either the ceiling is set so a maximal run fits (32 entries, under +any of the templates) or the cache bound is raised alongside it; the +proposal picks **32 and leaves the cache alone** unless a real template +needs more. + +The larger consequence is that a list-driven template's cost is set by the +caller: `list_workflows` quotes a per-workflow figure today, and a six-entry +`shots` list is six times the shot cost. The catalog entry should carry the +per-entry cost and the variable it multiplies by, **and the shape of an +entry** - which fields it takes, which are required, what `name` is for - +because an agent composing over MCP sees only the catalog and cannot +otherwise author the list. The MCP guidance (`docs/WORKFLOW_GUIDE.md`, +"Authoring a workflow from an agent") should say to quote `cost x len(list)` +before running. ### What this does not do @@ -180,37 +247,48 @@ the same entry: That is the same answer the guide already gives for the cartesian rule ("write it as one step per pair"), and it is why the entry should be an object -rather than a string in any template that has more than a prompt to vary. Two -`for_each` lists on one step, or a `for_each` over a cartesian product, is out -of scope and probably always should be. +rather than a string in any template that has more than a prompt to vary. The +same-key sibling rule is the one place two *steps* are paired, and it pairs +them by name over one list rather than zipping two. Two `for_each` lists on +one step, or a `for_each` over a cartesian product, is out of scope and +probably always should be. ## Phasing -1. **Expansion pass + `item:` + `gather:`**, schema, validation ordering, the - 64-step ceiling, and the reserved bracket name. No template changes. This is - self-contained and testable without a GPU: the pass is a pure function from - definition to definition, so its tests are ordinary unit tests. +1. **Expansion pass + `item:` + `gather:`** with structured `item:` values + and the same-key sibling rule, entry-name validation, schema, validation + ordering (arguments folded, the two directed errors), the 32-entry + ceiling, `release_pipeline`/`release_models` on the last member, and the + reserved `@`. No template changes. This is self-contained and testable + without a GPU: the pass is a pure function from definition to definition, + so its tests are ordinary unit tests - and the two tests that matter most + are the two templates rewritten by hand in the test suite and expanded + back to what they contain today. 2. **`music-video` and `dialogue-short` rewritten** onto a `shots` list. Both are breaking for scripted callers - the per-shot variables (`shot_2_deflect`, `shot_3_react`, …) become entries in one list - and T005 has already broken those names once this week, so the two should land together or the second should wait for a deliberate version bump of the + templates. The `dw:minimax-h3` plugin skill quotes those variables and + `tests/test_plugin_skills.py` pins what it quotes, so both move with the templates. -3. **Cost reporting** for a list-driven entry, in the catalog and in the - plugin skills that quote it. +3. **Cost and entry-shape reporting** for a list-driven entry, in the catalog + and in the plugin skills that quote it. Stage 1 is the bulk of the work and carries the risk; stages 2 and 3 are mechanical once it exists. -## Open questions for the approval - -- **Is the loop index needed inside an entry?** Nothing in the two templates - needs "shot 3 of 6" in a prompt, so v1 exposes no index. Adding `item:@index` - later is compatible; making it available now costs a second special name. -- **Should `for_each` accept a number** (`"for_each": 4`) for the case where - only the count varies? It reads well and it is one line in the pass, but it - gives entries no names, which is exactly the cache problem above. -- **`dialogue-short`'s two-speaker structure** (which character speaks in which - shot, T010's voice references) becomes a field on each entry - (`"speaker": "a"`). That is a template question, not an engine one, but it is - the thing stage 2 has to get right. +## Decided at review + +- **No loop index inside an entry.** Nothing in the two templates needs + "shot 3 of 6" in a prompt. Adding `item:@index` later is compatible; making + it available now costs a second special name. +- **`for_each` does not accept a number** (`"for_each": 4`). It reads well + and it is one line in the pass, but it gives entries no names, which is + exactly the cache problem above. +- **`dialogue-short`'s two-speaker structure** (which character speaks in + which shot, T010's voice references) is carried by each entry's + `references` list, spliced whole by `item:references` - a `"speaker": "a"` + flag was the first idea and cannot produce a references list whose length + varies by shot. The template question stage 2 has to get right is how much + of that list the entry writes and how much the template fixes. diff --git a/docs/superpowers/plans/2026-09-11-list-driven-steps.md b/docs/superpowers/plans/2026-09-11-list-driven-steps.md new file mode 100644 index 00000000..ebb6397a --- /dev/null +++ b/docs/superpowers/plans/2026-09-11-list-driven-steps.md @@ -0,0 +1,1632 @@ +# List-Driven Steps (`for_each`) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** A step can carry `"for_each": ` and expand into one ordinary step per entry before the run starts, so a template takes a `shots` list instead of one hand-written step per shot. + +**Architecture:** Expansion is a source transform, not a runtime concept. A new module `dw/for_each.py` holds one pure function, `expand_for_each(definition) -> definition`, that runs immediately after `replace_variables` in `Workflow.run` and, with the caller's arguments folded in, inside `Workflow.validation_errors`. After the pass the definition is an ordinary workflow: `item:` is substituted, `gather:group` is rewritten to an explicit `previous_result:` list, a same-key sibling reference is rewritten to the member with this key, and every member is named `group@key`. Nothing downstream - the step loop, the step cache, the manifest, the reference checker - learns a new reference kind. + +**Tech Stack:** Python 3, pytest. No new dependencies. + +**Spec:** `docs/proposals/list-driven-steps.md` (revised 2026-09-11). This plan is stage 1 of that proposal plus the agent-facing documentation; the template rewrite (stage 2) and catalog cost/entry-shape reporting (stage 3) are a later plan. + +## Global Constraints + +- Member separator is `@`; `@` is reserved in every step name, so a hand-written step name containing `@` is an error. +- Entry `name` must match `^[a-zA-Z_][a-zA-Z0-9_-]*$` (the `validate_variable_name` pattern in `dw/security.py`) and be unique within its list; an entry without a `name` (or a non-object entry) is keyed by its index. +- Ceiling: `MAX_FOR_EACH_ENTRIES = 32`; a longer list is refused with an error naming the ceiling. +- `for_each` runs over exactly one list; no index (`item:@index`), no numeric `for_each`, no zip. +- `item:field` yields whatever the field holds - string, number, list, object - spliced in as definition text. Bare `item:` is the whole entry. +- `gather:group` written inside a list splices into it; written as a scalar value it becomes a list. `gather:` naming a step that is not an earlier `for_each` group is an error. +- Inside a member, `previous_result:other` / `from_previous_result: "other"` naming an *earlier* `for_each` group over the *same* list (deep-equal) rewrites to `other@`. Naming a group any other way (from outside a group, from a group over a different list, or one's own group) is an error that says to use `gather:`. +- `release_pipeline` and `release_models` on a `for_each` step survive on the last member only. +- The realized workflow (`dw/realize.py`) keeps `for_each`; the manifest and events name the expanded members. No change to `dw/realize.py` is needed - it works on `self.workflow_definition`, which the pass never touches. +- Never use `eval()`, `exec()`, or `shell=True`. Errors carry a JSON path in the `steps[3].pipeline.arguments.prompt` shape `_render_path` in `dw/previous_results.py` produces. +- Run the suite with `python -m pytest tests -q -x`; commit after each task with the trailer `Co-Authored-By: Claude Fable 5.1 `. + +--- + +## File structure + +| File | Responsibility | +|---|---| +| `dw/for_each.py` (create) | The pass: constants, `ForEachError`, `expand_for_each`, `for_each_errors`. Pure; no I/O, no engine imports beyond `dw.security.validate_variable_name`, `dw.step_cache.reference_resolves_to` and the two constants from `dw.arguments`. | +| `dw/workflow.py` (modify) | Call the pass in `run` after substitution; `validation_errors(arguments=None)` substitutes, expands, then reference-checks the expanded definition. | +| `dw/workflow_schema.json` (modify) | `for_each` on a step. | +| `dw/server/app.py` (modify) | `validate_workflow` passes `request.arguments` into `validation_errors`. | +| `tests/test_for_each.py` (create) | Unit tests for the pass, including the two templates expanded back to what they contain today. | +| `tests/test_workflow.py`, `tests/test_validate_arguments.py` (modify) | Integration: run-time expansion, validation ordering. | +| `docs/WORKFLOW_GUIDE.md`, `CLAUDE.md`, `docs/SERVER.md` (modify) | The agent-facing conventions, the CLAUDE.md mirror, the manifest naming note. | + +--- + +### Task 1: The pass - names, ceiling, `item:` substitution + +**Files:** +- Create: `dw/for_each.py` +- Test: `tests/test_for_each.py` + +**Interfaces:** +- Consumes: `validate_variable_name` (`dw/security.py:390`, raises `InvalidInputError`), `reference_resolves_to` (`dw/step_cache.py:58`), `FROM_PREVIOUS_RESULT_KEY`, `PREVIOUS_RESULT_PREFIX` (`dw/arguments.py:49,58`). +- Produces: `expand_for_each(definition: dict) -> dict` (new dict, input untouched), `class ForEachError(ValueError)` with `.path: str`, constants `FOR_EACH_KEY = "for_each"`, `ITEM_PREFIX = "item:"`, `GATHER_PREFIX = "gather:"`, `MEMBER_SEPARATOR = "@"`, `MAX_FOR_EACH_ENTRIES = 32`, and `member_name(group: str, key: str) -> str`. Tasks 2 and 3 extend this module; Tasks 4-6 call it. + +- [ ] **Step 1: Write the failing tests** + +```python +# tests/test_for_each.py +import copy + +import pytest + +from dw.for_each import ( + MAX_FOR_EACH_ENTRIES, + ForEachError, + expand_for_each, + member_name, +) + + +def definition(*steps, **extra): + return {"id": "test", "steps": list(steps), **extra} + + +class TestNaming: + def test_member_name_joins_with_at(self): + assert member_name("shot", "wide_open") == "shot@wide_open" + + def test_an_object_entry_is_named_by_its_name(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "wide_open"}, {"name": "closeup"}], + "task": {"command": "x", "arguments": {}}, + } + ) + ) + assert [s["name"] for s in expanded["steps"]] == [ + "shot@wide_open", + "shot@closeup", + ] + + def test_a_nameless_entry_is_named_by_its_index(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": ["a", "b"], "task": {"arguments": {}}} + ) + ) + assert [s["name"] for s in expanded["steps"]] == ["shot@0", "shot@1"] + + def test_for_each_is_dropped_from_the_members(self): + expanded = expand_for_each( + definition({"name": "shot", "for_each": ["a"], "task": {}}) + ) + assert "for_each" not in expanded["steps"][0] + + def test_the_input_is_not_mutated(self): + original = definition({"name": "shot", "for_each": ["a"], "task": {}}) + before = copy.deepcopy(original) + expand_for_each(original) + assert original == before + + def test_a_workflow_without_for_each_comes_back_equal(self): + original = definition({"name": "plain", "task": {"arguments": {"a": 1}}}) + assert expand_for_each(original) == original + + def test_a_hand_written_at_sign_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each(definition({"name": "shot@0", "task": {}})) + assert e.value.path == "steps[0].name" + assert "@" in str(e.value) + + def test_a_duplicate_entry_name_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "closeup"}, {"name": "closeup"}], + "task": {}, + } + ) + ) + assert e.value.path == "steps[0].for_each[1].name" + assert "closeup" in str(e.value) + + def test_an_invalid_entry_name_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": [{"name": "wide open"}], "task": {}} + ) + ) + assert e.value.path == "steps[0].for_each[0].name" + + def test_a_non_list_for_each_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each(definition({"name": "shot", "for_each": 4, "task": {}})) + assert e.value.path == "steps[0].for_each" + assert "list" in str(e.value) + + def test_an_unsubstituted_variable_is_an_error_that_says_so(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "shot", "for_each": "variable:shots", "task": {}}) + ) + assert "variable:shots" in str(e.value) + + def test_an_empty_list_expands_to_no_steps(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": [], "task": {}}, + {"name": "after", "task": {}}, + ) + ) + assert [s["name"] for s in expanded["steps"]] == ["after"] + + def test_the_ceiling_is_enforced(self): + entries = [{"name": f"s{i}"} for i in range(MAX_FOR_EACH_ENTRIES + 1)] + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "shot", "for_each": entries, "task": {}}) + ) + assert str(MAX_FOR_EACH_ENTRIES) in str(e.value) + + def test_a_list_at_the_ceiling_is_fine(self): + entries = [{"name": f"s{i}"} for i in range(MAX_FOR_EACH_ENTRIES)] + expanded = expand_for_each( + definition({"name": "shot", "for_each": entries, "task": {}}) + ) + assert len(expanded["steps"]) == MAX_FOR_EACH_ENTRIES + + +class TestItemSubstitution: + def test_a_field_is_substituted(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a", "prompt": "the band walks on"}], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + assert expanded["steps"][0]["pipeline"]["arguments"]["prompt"] == ( + "the band walks on" + ) + + def test_a_bare_item_is_the_whole_entry(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": ["first prompt", "second prompt"], + "pipeline": {"arguments": {"prompt": "item:"}}, + } + ) + ) + prompts = [s["pipeline"]["arguments"]["prompt"] for s in expanded["steps"]] + assert prompts == ["first prompt", "second prompt"] + + def test_a_structured_field_is_spliced_whole(self): + references = [ + {"reference_type": "T", "from_previous_result": "draw_a"}, + {"reference_type": "T", "from_previous_result": "draw_b"}, + ] + expanded = expand_for_each( + definition( + {"name": "draw_a", "task": {}}, + {"name": "draw_b", "task": {}}, + { + "name": "shot", + "for_each": [{"name": "open", "references": references}], + "pipeline": {"arguments": {"references": "item:references"}}, + }, + ) + ) + assert expanded["steps"][2]["pipeline"]["arguments"]["references"] == references + + def test_a_number_keeps_its_type(self): + expanded = expand_for_each( + definition( + { + "name": "slice", + "for_each": [{"name": "a", "start_frame": 124}], + "task": {"arguments": {"start_frame": "item:start_frame"}}, + } + ) + ) + assert expanded["steps"][0]["task"]["arguments"]["start_frame"] == 124 + + def test_item_is_substituted_at_any_depth(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a", "voice": "asset:a.wav"}], + "pipeline": { + "arguments": {"references": [{"from_file": "item:voice"}]} + }, + } + ) + ) + assert expanded["steps"][0]["pipeline"]["arguments"]["references"] == [ + {"from_file": "asset:a.wav"} + ] + + def test_a_missing_field_is_an_error_at_its_path(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + assert e.value.path == "steps[0].pipeline.arguments.prompt" + assert "prompt" in str(e.value) and "a" in str(e.value) + + def test_a_field_of_a_string_entry_is_an_error(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + { + "name": "shot", + "for_each": ["just a prompt"], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + + def test_item_outside_a_for_each_step_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "plain", "task": {"arguments": {"p": "item:x"}}}) + ) + assert e.value.path == "steps[0].task.arguments.p" + + def test_members_do_not_share_mutable_values(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}, {"name": "b"}], + "pipeline": {"arguments": {"references": [{"k": 1}]}}, + } + ) + ) + first, second = expanded["steps"] + first["pipeline"]["arguments"]["references"][0]["k"] = 2 + assert second["pipeline"]["arguments"]["references"][0]["k"] == 1 + + +class TestRelease: + def test_release_flags_survive_on_the_last_member_only(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}, {"name": "b"}, {"name": "c"}], + "release_pipeline": True, + "release_models": True, + "pipeline": {}, + } + ) + ) + flags = [ + (s.get("release_pipeline"), s.get("release_models")) + for s in expanded["steps"] + ] + assert flags == [(None, None), (None, None), (True, True)] +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `python -m pytest tests/test_for_each.py -q` +Expected: FAIL with `ModuleNotFoundError: No module named 'dw.for_each'` + +- [ ] **Step 3: Write the module** + +```python +# dw/for_each.py +"""Expand a step's 'for_each' list into one ordinary step per entry. + +A template that generates one shot per entry of a list is otherwise written +the long way - 'shot_1', 'shot_2', ... each a near-copy of the one before - +and a six-shot episode is a different file from a five-shot one. This pass +runs on the definition after variable substitution and before the run id is +computed (Workflow.run) and before the reference check (validation_errors), +and produces a definition with no 'for_each' in it: every member is an +ordinary step, so the step loop, the step cache, the manifest and the +reference checker never learn a new reference kind. + +Inside a member: + - 'item:' is the whole entry; 'item:field' one field of an object + entry, spliced in whole whatever its type + - a reference to another for_each group over the SAME list resolves to + the member with the same key ('slice' inside 'shot@open' -> 'slice@open') +Outside (or inside, for any other group): + - 'gather:shot' is the list of every member's result, as explicit + 'previous_result:shot@' strings; inside a list it splices + - 'previous_result:shot' naming a group is an error that says to gather + +Members are named '@' - the entry's own 'name' when it carries +one, else its index - and '@' is reserved in every step name. Names rather +than indexes because the step cache (dw/step_cache.py) keys on the step +name: inserting a shot in the middle of a list must not shift every later +member onto a different entry's cache line. +""" + +import copy +import re + +from .arguments import FROM_PREVIOUS_RESULT_KEY, PREVIOUS_RESULT_PREFIX +from .security import InvalidInputError, validate_variable_name +from .step_cache import reference_resolves_to + +FOR_EACH_KEY = "for_each" +ITEM_PREFIX = "item:" +GATHER_PREFIX = "gather:" +MEMBER_SEPARATOR = "@" +# Each entry is a full generation. Stated against the step cache's bound +# (DEFAULT_MAX_ENTRIES = 50): a run whose expanded steps exceed the cache +# evicts its own earlier members, so this is kept well under it +MAX_FOR_EACH_ENTRIES = 32 +# release_pipeline / release_models would drop the model after the first +# member and reload it for the second, so they are carried onto the last one +_LAST_MEMBER_ONLY = ("release_pipeline", "release_models") + + +class ForEachError(ValueError): + """A for_each step that cannot be expanded, with the JSON path at fault.""" + + def __init__(self, path, message): + super().__init__(message) + self.path = path + + +def member_name(group, key): + return f"{group}{MEMBER_SEPARATOR}{key}" + + +def expand_for_each(definition): + """The definition with every 'for_each' step replaced by its members. + + Returns a new structure; `definition` is left as it was passed in. + Raises ForEachError for anything that cannot be expanded. + """ + steps = definition.get("steps") if isinstance(definition, dict) else None + if not isinstance(steps, list): + return definition + + # group name -> {"keys": [...], "entries": [...]} for every group + # expanded so far, in step order, so a reference can only reach an + # earlier group - the same rule previous_result: has always had + groups = {} + expanded = [] + for index, step in enumerate(steps): + if not isinstance(step, dict): + expanded.append(copy.deepcopy(step)) + continue + path = ("steps", index) + name = step.get("name") + if isinstance(name, str) and MEMBER_SEPARATOR in name: + raise ForEachError( + _render_path(path + ("name",)), + f"Step name '{name}' contains '{MEMBER_SEPARATOR}', which is " + f"reserved for the members of a for_each step", + ) + if FOR_EACH_KEY not in step: + expanded.append(_rewrite(step, path, groups, member=None)) + continue + + entries = step[FOR_EACH_KEY] + keys = _entry_keys(entries, path + (FOR_EACH_KEY,)) + template = {k: v for k, v in step.items() if k != FOR_EACH_KEY} + last = len(keys) - 1 + for position, (key, entry) in enumerate(zip(keys, entries)): + member = { + "group": name, + "key": key, + "entry": entry, + "entries": entries, + "index": position, + } + expanded_step = _rewrite(template, path, groups, member) + expanded_step["name"] = member_name(name, key) + if position != last: + for flag in _LAST_MEMBER_ONLY: + expanded_step.pop(flag, None) + expanded.append(expanded_step) + groups[name] = {"keys": keys, "entries": entries} + + result = {k: v for k, v in definition.items() if k != "steps"} + result["steps"] = expanded + return result + + +def _entry_keys(entries, path): + """The key of every entry - its 'name' when it is an object carrying + one, else its index - validated and unique.""" + if not isinstance(entries, list): + if isinstance(entries, str) and entries.startswith("variable:"): + hint = f" - '{entries}' was not substituted; is the variable declared?" + else: + hint = "" + raise ForEachError( + _render_path(path), f"for_each must be a list, got {type(entries).__name__}{hint}" + ) + if len(entries) > MAX_FOR_EACH_ENTRIES: + raise ForEachError( + _render_path(path), + f"for_each has {len(entries)} entries; the limit is {MAX_FOR_EACH_ENTRIES}", + ) + keys = [] + for index, entry in enumerate(entries): + key = str(index) + if isinstance(entry, dict) and "name" in entry: + key = entry["name"] + key_path = _render_path(path + (index, "name")) + if not isinstance(key, str): + raise ForEachError(key_path, "An entry's name must be a string") + try: + validate_variable_name(key) + except InvalidInputError as e: + raise ForEachError(key_path, f"Invalid entry name '{key}': {e}") from e + if key in keys: + raise ForEachError(key_path, f"Duplicate entry name '{key}'") + keys.append(key) + return keys + + +def _rewrite(value, path, groups, member): + """Rebuild `value` with item:, gather: and group references resolved. + + `member` is None outside a for_each step; inside one it carries the + group, key and entry of the member being built. + """ + if isinstance(value, dict): + rebuilt = {} + for key, item in value.items(): + if key == FROM_PREVIOUS_RESULT_KEY and isinstance(item, str): + rebuilt[key] = _rewrite_reference(item, path + (key,), groups, member) + else: + rebuilt[key] = _rewrite(item, path + (key,), groups, member) + return rebuilt + if isinstance(value, list): + rebuilt = [] + for index, item in enumerate(value): + if isinstance(item, str) and item.startswith(GATHER_PREFIX): + # A gather inside a list splices into it + rebuilt.extend(_gather(item, path + (index,), groups)) + else: + rebuilt.append(_rewrite(item, path + (index,), groups, member)) + return rebuilt + if isinstance(value, str): + if value.startswith(GATHER_PREFIX): + return _gather(value, path, groups) + if value.startswith(ITEM_PREFIX): + return _item(value, path, member) + if value.startswith(PREVIOUS_RESULT_PREFIX): + reference = value[len(PREVIOUS_RESULT_PREFIX) :] + return PREVIOUS_RESULT_PREFIX + _rewrite_reference( + reference, path, groups, member + ) + return value + return copy.deepcopy(value) + + +def _item(value, path, member): + if member is None: + raise ForEachError( + _render_path(path), f"'{value}' is only meaningful inside a for_each step" + ) + field = value[len(ITEM_PREFIX) :] + entry = member["entry"] + if field == "": + return copy.deepcopy(entry) + if not isinstance(entry, dict): + raise ForEachError( + _render_path(path), + f"'{value}' asks for a field of entry '{member['key']}' of " + f"for_each step '{member['group']}', which is not an object", + ) + if field not in entry: + raise ForEachError( + _render_path(path), + f"'{value}' names no field of entry '{member['key']}' of for_each " + f"step '{member['group']}'; it has: {sorted(entry)}", + ) + return copy.deepcopy(entry[field]) + + +def _gather(value, path, groups): + group = value[len(GATHER_PREFIX) :] + if group not in groups: + raise ForEachError( + _render_path(path), + f"'{value}' names no earlier for_each step. " + f"for_each steps available here: {sorted(groups)}", + ) + return [ + PREVIOUS_RESULT_PREFIX + member_name(group, key) + for key in groups[group]["keys"] + ] + + +def _rewrite_reference(reference, path, groups, member): + """A previous_result reference (without its prefix) as the expanded + definition spells it: unchanged unless it names a for_each group.""" + if reference.startswith("variable:"): + return reference + group = next((g for g in groups if reference_resolves_to(reference, g)), None) + if group is None: + if member is not None and reference_resolves_to(reference, member["group"]): + raise ForEachError( + _render_path(path), + f"'{reference}' names its own for_each step '{member['group']}'", + ) + return reference + if member is not None and member["entries"] == groups[group]["entries"]: + # Same list: shot@open reads slice@open + return member_name(group, member["key"]) + reference[len(group) :] + where = ( + f"from inside for_each step '{member['group']}', which runs over a different list" + if member is not None + else "from outside a for_each step" + ) + raise ForEachError( + _render_path(path), + f"'{reference}' names the for_each step '{group}' {where}. Use " + f"'{GATHER_PREFIX}{group}' for every member's result, or a reference " + f"from a for_each step over the same list for the same-keyed member", + ) + + +def _render_path(path): + """'steps[3].task.arguments.videos[1]' - the same shape schema errors use.""" + rendered = "" + for part in path: + if isinstance(part, int): + rendered += f"[{part}]" + elif rendered: + rendered += f".{part}" + else: + rendered = str(part) + return rendered +``` + +Note: `_render_path` duplicates `dw/previous_results.py:_render_path`; move that one here and import it from `previous_results` (`from .for_each import _render_path` would be circular the other way - `for_each` must not import `previous_results`). Do the move: delete the copy in `previous_results.py` and add `from .for_each import _render_path` there, renaming it `render_path` (public) in both places. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `python -m pytest tests/test_for_each.py tests/test_previous_results.py -q` +Expected: all PASS + +- [ ] **Step 5: Commit** + +```bash +git add dw/for_each.py dw/previous_results.py tests/test_for_each.py +git commit -m "feat(for_each): expansion pass - member naming, ceiling, item: substitution + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 2: `gather:` and group references + +**Files:** +- Modify: `dw/for_each.py` (already written in Task 1; this task tests the `_gather` / `_rewrite_reference` behaviour and fixes what the tests find) +- Test: `tests/test_for_each.py` + +**Interfaces:** +- Consumes: Task 1's module. +- Produces: the same `expand_for_each`; behaviour now pinned for `gather:`, same-key siblings and the directed errors. + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/test_for_each.py`: + +```python +class TestGather: + def group(self, *extra): + return definition( + { + "name": "shot", + "for_each": [{"name": "open"}, {"name": "close"}], + "pipeline": {"arguments": {}}, + }, + *extra, + ) + + def test_a_scalar_gather_becomes_the_member_list(self): + expanded = expand_for_each( + self.group( + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}} + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [ + "previous_result:shot@open", + "previous_result:shot@close", + ] + + def test_a_gather_inside_a_list_splices(self): + expanded = expand_for_each( + self.group( + {"name": "intro", "task": {}}, + { + "name": "edit", + "task": { + "arguments": { + "videos": ["previous_result:intro", "gather:shot"] + } + }, + }, + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [ + "previous_result:intro", + "previous_result:shot@open", + "previous_result:shot@close", + ] + + def test_gather_of_an_empty_group_is_an_empty_list(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": [], "task": {}}, + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}}, + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [] + + def test_gather_of_a_plain_step_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "one", "task": {}}, + {"name": "edit", "task": {"arguments": {"videos": "gather:one"}}}, + ) + ) + assert e.value.path == "steps[1].task.arguments.videos" + assert "no earlier for_each step" in str(e.value) + + def test_gather_of_a_later_group_is_an_error(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}}, + {"name": "shot", "for_each": ["a"], "task": {}}, + ) + ) + + def test_gather_in_a_sub_workflow_argument_map(self): + expanded = expand_for_each( + self.group( + { + "name": "score", + "workflow": {"path": "builtin:x.json", "arguments": {"clips": "gather:shot"}}, + } + ) + ) + assert expanded["steps"][-1]["workflow"]["arguments"]["clips"] == [ + "previous_result:shot@open", + "previous_result:shot@close", + ] + + +class TestGroupReferences: + def test_previous_result_naming_a_group_says_to_gather(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": {"arguments": {"video": "previous_result:shot"}}, + }, + ) + ) + assert e.value.path == "steps[1].task.arguments.video" + assert "gather:shot" in str(e.value) + + def test_from_previous_result_naming_a_group_says_to_gather(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": { + "arguments": {"refs": [{"from_previous_result": "shot"}]} + }, + }, + ) + ) + assert e.value.path == "steps[1].task.arguments.refs[0].from_previous_result" + + def test_a_property_reference_to_a_group_is_also_refused(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": {"arguments": {"v": "previous_result:shot.frames"}}, + }, + ) + ) + + def test_a_reference_to_an_ordinary_step_is_untouched(self): + expanded = expand_for_each( + definition( + {"name": "draw", "task": {}}, + { + "name": "shot", + "for_each": ["a"], + "pipeline": { + "arguments": { + "references": [{"from_previous_result": "draw"}], + "still": "previous_result:draw.image", + } + }, + }, + ) + ) + arguments = expanded["steps"][1]["pipeline"]["arguments"] + assert arguments["references"] == [{"from_previous_result": "draw"}] + assert arguments["still"] == "previous_result:draw.image" + + def test_a_variable_spelled_from_previous_result_is_untouched(self): + expanded = expand_for_each( + definition( + { + "name": "edit", + "task": {"arguments": {"r": [{"from_previous_result": "variable:x"}]}}, + } + ) + ) + assert expanded["steps"][0]["task"]["arguments"]["r"] == [ + {"from_previous_result": "variable:x"} + ] + + +class TestSameKeySiblings: + def shots(self): + return [ + {"name": "open", "start_frame": 0}, + {"name": "close", "start_frame": 124}, + ] + + def test_a_sibling_over_the_same_list_resolves_to_the_same_key(self): + shots = self.shots() + expanded = expand_for_each( + definition( + { + "name": "slice", + "for_each": shots, + "task": {"arguments": {"start_frame": "item:start_frame"}}, + }, + { + "name": "shot", + "for_each": shots, + "pipeline": { + "arguments": { + "references": [{"from_previous_result": "slice"}], + "audio": "previous_result:slice.audio", + } + }, + }, + ) + ) + names = [s["name"] for s in expanded["steps"]] + assert names == ["slice@open", "slice@close", "shot@open", "shot@close"] + close = expanded["steps"][3]["pipeline"]["arguments"] + assert close["references"] == [{"from_previous_result": "slice@close"}] + assert close["audio"] == "previous_result:slice@close.audio" + + def test_the_same_list_means_equal_not_identical(self): + expanded = expand_for_each( + definition( + {"name": "slice", "for_each": self.shots(), "task": {}}, + { + "name": "shot", + "for_each": self.shots(), + "task": {"arguments": {"a": "previous_result:slice"}}, + }, + ) + ) + assert expanded["steps"][2]["task"]["arguments"]["a"] == ( + "previous_result:slice@open" + ) + + def test_a_sibling_over_a_different_list_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "slice", "for_each": ["a", "b"], "task": {}}, + { + "name": "shot", + "for_each": ["a"], + "task": {"arguments": {"x": "previous_result:slice"}}, + }, + ) + ) + assert "different list" in str(e.value) + + def test_a_step_referencing_its_own_group_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": ["a", "b"], + "task": {"arguments": {"x": "previous_result:shot"}}, + } + ) + ) + assert "its own" in str(e.value) + + def test_a_gather_inside_a_member_still_gathers(self): + expanded = expand_for_each( + definition( + {"name": "slice", "for_each": ["a", "b"], "task": {}}, + { + "name": "shot", + "for_each": ["x"], + "task": {"arguments": {"all": "gather:slice"}}, + }, + ) + ) + assert expanded["steps"][2]["task"]["arguments"]["all"] == [ + "previous_result:slice@0", + "previous_result:slice@1", + ] +``` + +- [ ] **Step 2: Run the tests** + +Run: `python -m pytest tests/test_for_each.py -q` +Expected: the new classes PASS against the Task 1 module. If any fails, fix `_gather` / `_rewrite_reference` in `dw/for_each.py` until they pass - the tests are the specification, the code is not. + +- [ ] **Step 3: Commit** + +```bash +git add dw/for_each.py tests/test_for_each.py +git commit -m "test(for_each): gather:, same-key siblings and the directed group errors + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 3: The two templates, expanded back to what they contain today + +This is the test that proves stage 2 is mechanical. It writes each template's shot section the list-driven way *inside the test* and asserts the expansion equals the hand-written steps that exist now. + +**Files:** +- Test: `tests/test_for_each.py` +- Read: `workflows/templates/minimax/music-video.json`, `workflows/templates/minimax/dialogue-short.json` + +**Interfaces:** +- Consumes: `expand_for_each`. +- Produces: nothing new; a regression guard for stage 2. + +- [ ] **Step 1: Write the test for `music-video`** + +Append to `tests/test_for_each.py`: + +```python +import json +import os + +TEMPLATES = os.path.join( + os.path.dirname(__file__), "..", "workflows", "templates", "minimax" +) + + +def load_template(name): + with open(os.path.join(TEMPLATES, name)) as f: + return json.load(f) + + +def steps_by_name(definition): + return {s["name"]: s for s in definition["steps"]} + + +def without_pipeline_reference(step, pipeline_step): + """A hand-written 'pipeline_reference' shot as the full pipeline block + the expansion produces: the reference's arguments over the referenced + step's pipeline.""" + rebuilt = {k: v for k, v in step.items() if k != "pipeline_reference"} + pipeline = copy.deepcopy(pipeline_step["pipeline"]) + pipeline["arguments"] = step["pipeline_reference"]["arguments"] + rebuilt["pipeline"] = pipeline + return rebuilt + + +class TestMusicVideoTemplate: + """music-video's four slices and four shots, written as two for_each + groups over one 'shots' list, expand to the steps the template holds + by hand today.""" + + def test_the_hand_written_shots_are_what_the_list_expands_to(self): + template = load_template("music-video.json") + today = steps_by_name(template) + shots = [ + {"name": "wide_open", "prompt": "variable:shot_1_wide_open", "start_frame": 0}, + {"name": "closeup", "prompt": "variable:shot_2_closeup", "start_frame": 124}, + {"name": "room", "prompt": "variable:shot_3_room", "start_frame": 248}, + {"name": "finale", "prompt": "variable:shot_4_finale", "start_frame": 372}, + ] + slice_template = copy.deepcopy(today["slice_1"]) + slice_template["name"] = "slice" + slice_template["for_each"] = shots + slice_template["task"]["arguments"]["start_frame"] = "item:start_frame" + + shot_template = copy.deepcopy(today["shot_1_wide_open"]) + shot_template["name"] = "shot" + shot_template["for_each"] = shots + shot_template["pipeline"]["arguments"]["prompt"] = "item:prompt" + for reference in shot_template["pipeline"]["arguments"]["references"]: + if reference.get("from_previous_result") == "slice_1": + reference["from_previous_result"] = "slice" + + edit = copy.deepcopy(today["edit"]) + edit["task"]["arguments"]["videos"] = "gather:shot" + + expanded = expand_for_each( + definition( + today["draw_singer"], today["write_song"], slice_template, + today["soundtrack"], shot_template, edit, today["music_video"], + ) + ) + got = steps_by_name(expanded) + + # Each expanded slice is today's slice with the new name + for key, old in zip(["wide_open", "closeup", "room", "finale"], range(1, 5)): + expected = copy.deepcopy(today[f"slice_{old}"]) + expected["name"] = f"slice@{key}" + assert got[f"slice@{key}"] == expected + + # Each expanded shot is today's shot (as a full pipeline block) with + # the new name and its slice renamed + hand_written = ["shot_1_wide_open", "shot_2_closeup", "shot_3_room", "shot_4_finale"] + for key, old, index in zip(["wide_open", "closeup", "room", "finale"], hand_written, range(1, 5)): + step = today[old] + if "pipeline_reference" in step: + step = without_pipeline_reference(step, today["shot_1_wide_open"]) + expected = copy.deepcopy(step) + expected["name"] = f"shot@{key}" + for reference in expected["pipeline"]["arguments"]["references"]: + if reference.get("from_previous_result") == f"slice_{index}": + reference["from_previous_result"] = f"slice@{key}" + assert got[f"shot@{key}"] == expected + + assert got["edit"]["task"]["arguments"]["videos"] == [ + f"previous_result:shot@{k}" for k in ["wide_open", "closeup", "room", "finale"] + ] +``` + +- [ ] **Step 2: Run it, and adjust the test to the template's real shape** + +Run: `python -m pytest tests/test_for_each.py::TestMusicVideoTemplate -q` + +The test was written from the template as of 2026-09-11 (`slice_1..4` with `start_frame` 0/124/248/372; shots 2-4 as `pipeline_reference` to `shot_1_wide_open`; `edit.videos` the four shots). If the assertion fails on a field this plan did not anticipate (a `pipeline_reference` carrying more than `arguments`, say), read the diff pytest prints and fix the *test's* reconstruction so it mirrors the template - the expansion is not to be bent to a template. If it fails because the expansion is wrong, fix `dw/for_each.py`. + +Expected after adjustment: PASS + +- [ ] **Step 3: Write the test for `dialogue-short`** + +Append: + +```python +class TestDialogueShortTemplate: + """dialogue-short's five shots, whose reference lists differ in length + by shot, written as one for_each group whose entries carry the whole + references list.""" + + def test_the_hand_written_shots_are_what_the_list_expands_to(self): + template = load_template("dialogue-short.json") + today = steps_by_name(template) + hand_written = [ + ("cold_open", "shot_1_cold_open"), + ("deflect", "shot_2_deflect"), + ("react", "shot_3_react"), + ("button", "shot_4_button"), + ("tag", "shot_5_tag"), + ] + first = today["shot_1_cold_open"] + full = { + old: (step if "pipeline_reference" not in step else without_pipeline_reference(step, first)) + for old, step in today.items() + if old.startswith("shot_") + } + shots = [] + for key, old in hand_written: + arguments = full[old]["pipeline"]["arguments"] + entry = {"name": key, "prompt": arguments["prompt"], "references": arguments["references"]} + # Only the tag shot has its own frame count in the template + if arguments.get("num_frames") != first["pipeline"]["arguments"]["num_frames"]: + entry["num_frames"] = arguments["num_frames"] + shots.append(entry) + + shot_template = copy.deepcopy(full["shot_1_cold_open"]) + shot_template["name"] = "shot" + shot_template["for_each"] = shots + shot_template["pipeline"]["arguments"]["prompt"] = "item:prompt" + shot_template["pipeline"]["arguments"]["references"] = "item:references" + + expanded = expand_for_each( + definition(today["draw_character_a"], today["draw_character_b"], shot_template) + ) + got = steps_by_name(expanded) + for key, old in hand_written: + expected = copy.deepcopy(full[old]) + expected["name"] = f"shot@{key}" + if "num_frames" not in shots[[k for k, _ in hand_written].index(key)]: + # The member inherits the template's frame count, which is + # what the hand-written shot has too + pass + got_step = got[f"shot@{key}"] + assert got_step["pipeline"]["arguments"]["prompt"] == expected["pipeline"]["arguments"]["prompt"] + assert got_step["pipeline"]["arguments"]["references"] == expected["pipeline"]["arguments"]["references"] +``` + +Then read `dialogue-short.json` (`python3 -c "import json; d=json.load(open('workflows/templates/minimax/dialogue-short.json')); [print(s['name'], json.dumps(s)[:600]) for s in d['steps']]"`) and extend the entry to carry every argument that differs between shots (the template's `num_frames` vs `tag_num_frames` is one; there may be others), mapping each to an `item:` in `shot_template`. The final assertion should compare the *whole* step: `assert got[f"shot@{key}"] == expected`, with `expected["name"]` set as above. Replace the two field assertions with that once the entry shape is right. + +- [ ] **Step 4: Run both template tests** + +Run: `python -m pytest tests/test_for_each.py -q` +Expected: all PASS + +- [ ] **Step 5: Commit** + +```bash +git add tests/test_for_each.py +git commit -m "test(for_each): music-video and dialogue-short expand back to their hand-written steps + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 4: Schema - `for_each` on a step + +**Files:** +- Modify: `dw/workflow_schema.json` (the `step` definition, around line 100-135) +- Test: `tests/test_schema.py` + +**Interfaces:** +- Produces: a definition with `"for_each"` on a step validates; `for_each` accepts a list or a `variable:` string. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_schema.py`: + +```python +def test_for_each_is_a_list_or_a_variable_reference(): + schema = load_schema("workflow") + base = { + "id": "t", + "steps": [{"name": "shot", "task": {"command": "x"}, "for_each": None}], + } + for good in (["a", "b"], [{"name": "a"}], "variable:shots"): + base["steps"][0]["for_each"] = good + assert validate_data_all(base, schema) == [] + for bad in (4, "shots", {"a": 1}): + base["steps"][0]["for_each"] = bad + assert validate_data_all(base, schema) != [] +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `python -m pytest tests/test_schema.py::test_for_each_is_a_list_or_a_variable_reference -q` +Expected: FAIL (the `4` and `"shots"` cases validate today because the step allows unknown keys - check: if the test passes outright, the step has no `additionalProperties: false`; the test's `bad` loop is then the failing half). + +- [ ] **Step 3: Add the property** + +In `dw/workflow_schema.json`, inside `"step": {"properties": {...}}`, after `"name"`: + +```json +"for_each": { + "description": "Run this step once per entry of a list. A list, or a 'variable:' reference to one. Inside the step, 'item:' is the entry and 'item:field' one of its fields; a later step reads every member's result with 'gather:'. Members are named '@' (or '@' for an entry without a name), so '@' is reserved in step names.", + "type": ["array", "string"], + "pattern": "^variable:", + "maxItems": 32 +}, +``` + +`"pattern"` only applies to strings and `"maxItems"` only to arrays, which is how `seed` already mixes the two on the same key. + +- [ ] **Step 4: Run the schema tests** + +Run: `python -m pytest tests/test_schema.py tests/test_configuration_schema.py -q` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add dw/workflow_schema.json tests/test_schema.py +git commit -m "feat(schema): for_each on a step + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 5: Wire the pass into `Workflow.run` and `validation_errors` + +**Files:** +- Modify: `dw/workflow.py:286-296` (`validation_errors`), `dw/workflow.py:370-386` (the substitution block in `run`) +- Test: `tests/test_workflow.py` + +**Interfaces:** +- Consumes: `expand_for_each`, `ForEachError` from Task 1; `replace_variables`, `set_variables`, `VariableNotFoundError` from `dw/variables.py`; `argument_errors` from `dw/variables.py`. +- Produces: `Workflow.validation_errors(self, arguments=None)` - the new keyword is what Task 6 passes; `Workflow.expanded_definition(self, arguments=None) -> dict`. + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/test_workflow.py` (it already imports `Workflow`/`workflow_from_definition`-style helpers - follow the file's existing pattern for constructing a workflow from a dict, e.g. the one `test_validate_catches_a_reference_to_a_step_that_does_not_exist` at line 583 uses): + +```python +def _for_each_workflow(**overrides): + definition = { + "id": "fe", + "variables": {"shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}]}, + "steps": [ + { + "name": "shot", + "for_each": "variable:shots", + "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "result": {"content_type": "text/plain"}, + }, + { + "name": "edit", + "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "result": {"content_type": "text/plain"}, + }, + ], + } + definition.update(overrides) + return definition + + +def test_validation_expands_for_each_before_the_reference_check(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) # the file's helper + assert workflow.validation_errors() == [] + + +def test_validation_reports_a_for_each_error_at_its_path(tmp_path): + definition = _for_each_workflow() + definition["steps"][1]["task"]["arguments"]["parts"] = "previous_result:shot" + workflow = _workflow_from(definition, tmp_path) + errors = workflow.validation_errors() + assert len(errors) == 1 + assert errors[0]["path"] == "steps[1].task.arguments.parts" + assert "gather:shot" in errors[0]["message"] + + +def test_validation_expands_the_callers_list_not_the_default(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + # Two entries share a name only in the caller's list + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a"}, {"name": "a"}]} + ) + assert errors and "Duplicate entry name 'a'" in errors[0]["message"] + + +def test_validation_falls_back_to_the_default_when_the_arguments_are_bad(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + # An undeclared argument is argument_errors' finding, not validation's + assert workflow.validation_errors(arguments={"nope": 1}) == [] + + +def test_validation_of_an_undeclared_variable_still_does_not_raise(tmp_path): + definition = _for_each_workflow() + definition["steps"][0]["for_each"] = "variable:missing" + workflow = _workflow_from(definition, tmp_path) + errors = workflow.validation_errors() + assert errors and "variable:missing" in errors[0]["message"] + + +def test_run_expands_for_each_and_names_the_members(tmp_path): + workflow = _workflow_from(_for_each_workflow(seed=1), tmp_path) + workflow.run({}) + names = [entry["step"] for entry in workflow.manifest] + assert names == ["shot@a", "shot@b", "edit"] + + +def test_run_substitutes_the_callers_list(tmp_path): + workflow = _workflow_from(_for_each_workflow(seed=1), tmp_path) + workflow.run({"shots": [{"name": "only", "text": "X"}]}) + names = [entry["step"] for entry in workflow.manifest] + assert names == ["shot@only", "edit"] +``` + +Check the manifest entry's key for the step name (`grep -n "manifest.append" dw/workflow.py`) and use whatever key it carries in place of `entry["step"]`. + +- [ ] **Step 2: Run them to verify they fail** + +Run: `python -m pytest tests/test_workflow.py -k "for_each" -q` +Expected: FAIL - `validation_errors()` reports `gather:shot`/`item:` nothing but the run fails on `for_each` as an unknown step key, and `validation_errors(arguments=...)` is a TypeError. + +- [ ] **Step 3: Implement** + +In `dw/workflow.py` add the imports: + +```python +from .for_each import expand_for_each, ForEachError +from .variables import ( + argument_errors, + replace_variables, + set_variables, + VariableNotFoundError, +) +``` + +(merge with the existing `from .variables import replace_variables, set_variables` at line 45.) + +Replace `validation_errors`: + +```python + def expanded_definition(self, arguments=None): + """The definition as the run will see it: variables substituted - + the caller's `arguments` folded in when they are all good, else the + declared defaults - and every for_each step expanded. + + Raises ForEachError for a for_each that cannot be expanded. A + 'variable:' that names nothing is left in place rather than raised: + validate_workflow already reports that as a warning, and the + reference check is happy to skip a reference it cannot read. + """ + definition = copy.deepcopy(self.workflow_definition) + variables = definition.get("variables") + if isinstance(variables, dict): + if arguments and not argument_errors(definition, arguments): + set_variables(arguments, variables) + try: + definition = replace_variables(definition, variables) + except VariableNotFoundError: + pass + return expand_for_each(definition) + + def validation_errors(self, arguments=None): + """Every schema violation in the definition, as [{path, message}]; + empty when it validates. `arguments` are the caller's, so a + for_each over a list the caller supplies is checked as it will run.""" + errors = validate_data_all(self.workflow_definition, load_schema("workflow")) + # Only once the shape is known good: the passes below walk the + # steps array and a definition that fails the schema may have no + # such array to walk + if errors: + return errors + try: + expanded = self.expanded_definition(arguments) + except ForEachError as e: + return [{"path": e.path, "message": str(e)}] + return previous_result_reference_errors(expanded) +``` + +`set_variables` may need `realize_constants` first when a default is `constant:` - it does not for the list case, and `argument_errors` already runs `set_variables` on the raw declared block, so this mirrors it. + +In `run`, directly after `workflow_def = replace_variables(workflow_def, variables)` (line 386) and *outside* the `if variables is not None` block: + +```python + # One ordinary step per entry of every for_each list, before the + # seed, the run id and the realized workflow are computed, so + # each covers what actually runs. A ForEachError here fails the + # run before anything loads + workflow_def = expand_for_each(workflow_def) +``` + +Note: `steps = workflow_def["steps"]` (or wherever the loop reads them) must be read *after* this line - check with `grep -n "steps = " dw/workflow.py` and move the read below the expansion if it sits above. + +- [ ] **Step 4: Run the tests** + +Run: `python -m pytest tests/test_workflow.py tests/test_for_each.py tests/test_previous_results.py -q` +Expected: PASS. The `previous_result_reference_errors` behaviour changed subtly - it now runs on the *substituted* definition, so a `from_previous_result: "variable:x"` whose variable is declared is now checked by its resolved value. If an existing test pins the old skip, read it: the skip was for a value that "is not knowable before substitution", which a declared default now is. Update the test's expectation, not the code. + +- [ ] **Step 5: Run the whole suite** + +Run: `python -m pytest tests -q -x` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add dw/workflow.py tests/test_workflow.py +git commit -m "feat(for_each): expand in Workflow.run and validate the expanded definition + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 6: The server pre-flight validates with the caller's list + +**Files:** +- Modify: `dw/server/app.py:1270` (the `errors = candidate.validation_errors()` call in `validate_workflow`) +- Test: `tests/test_validate_arguments.py` + +**Interfaces:** +- Consumes: `Workflow.validation_errors(arguments=...)` from Task 5. + +- [ ] **Step 1: Write the failing test** + +Look at how `tests/test_validate_arguments.py` builds its client and posts to `/api/validate` (`grep -n "def test\|client.post" tests/test_validate_arguments.py | head`), then append in the same style: + +```python +def test_validate_expands_for_each_with_the_callers_list(client): + workflow = { + "id": "fe", + "variables": {"shots": [{"name": "a", "text": "A"}]}, + "steps": [ + { + "name": "shot", + "for_each": "variable:shots", + "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "result": {"content_type": "text/plain"}, + }, + { + "name": "edit", + "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "result": {"content_type": "text/plain"}, + }, + ], + } + ok = client.post("/api/validate", json={"workflow": workflow}).json() + assert ok["valid"] is True + + bad = client.post( + "/api/validate", + json={"workflow": workflow, "arguments": {"shots": [{"name": "x"}, {"name": "x"}]}}, + ).json() + assert bad["valid"] is False + assert bad["errors"][0]["path"] == "steps[0].for_each[1].name" +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `python -m pytest tests/test_validate_arguments.py -k for_each -q` +Expected: FAIL - the second call answers `valid: True` because the default list was expanded. + +- [ ] **Step 3: Pass the arguments through** + +In `dw/server/app.py`, change + +```python + errors = candidate.validation_errors() +``` + +to + +```python + # The caller's list is the one a for_each expands over, so the + # pre-flight checks the step set that will actually run + errors = candidate.validation_errors(arguments=request.arguments) +``` + +- [ ] **Step 4: Run the server tests** + +Run: `python -m pytest tests/test_validate_arguments.py tests/test_server_jobs.py -q` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add dw/server/app.py tests/test_validate_arguments.py +git commit -m "feat(server): POST /api/validate expands for_each over the caller's list + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 7: Documentation - the agent-facing conventions + +This is how an agent learns the feature. `get_guide("workflows", section=...)` serves `docs/WORKFLOW_GUIDE.md` by heading, and the MCP instructions send an agent to "Authoring a workflow from an agent" before it writes JSON; CLAUDE.md's Type System block is the mirror the repo keeps in sync by rule. + +**Files:** +- Modify: `docs/WORKFLOW_GUIDE.md` (the `### References` list at line 244, and a new subsection after `### Several \`previous_result\` references multiply`, line 318) +- Modify: `CLAUDE.md` (the Type System bullets, after the `prompt:` bullet at line 152; the Critical Gotchas list) +- Modify: `docs/SERVER.md` (where the manifest is described - `grep -n "manifest" docs/SERVER.md`) +- Test: `tests/test_server_guides.py` / `tests/test_mcp_guides.py` only if they pin section names (`grep -n "Authoring\|sections" tests/test_server_guides.py`); nothing to add otherwise. + +- [ ] **Step 1: Add the three prefixes to the References list in `docs/WORKFLOW_GUIDE.md`** + +After the `prompt:` bullet (ends "rather than resolving twice."), add: + +```markdown +- `item:` — only inside a step that carries `for_each`: `item:` is the + entry the member was made for, `item:field` one field of an object entry, + spliced in whole whatever its type — a string, a number, a list of + references. See "One step per entry" below. +- `gather:` — `gather:shot` is the result of *every* member of the + `for_each` step `shot`, in list order, as one list. Inside a list it splices + into it. It is how a step downstream of a fan-out reads the whole group; + `previous_result:shot` naming a `for_each` step is an error that says so. +``` + +- [ ] **Step 2: Add the subsection** + +After the paragraph ending "the signal to restructure the workflow, not to add another reference." and before `### The loop`, add: + +```markdown +### One step per entry: `for_each` + +A step that carries `for_each` runs once per entry of a list — a shot per +entry of `shots` — and the list is a variable the caller supplies, so a +six-shot episode is an argument rather than a different file. + +```json +{ + "name": "shot", + "for_each": "variable:shots", + "pipeline": { + "arguments": { + "prompt": "item:prompt", + "references": "item:references" + } + } +} +``` + +with + +```json +"shots": [ + { "name": "wide_open", "prompt": "the band walks on, wide", + "references": [{ "reference_type": "…", "from_previous_result": "draw_singer" }] }, + { "name": "closeup", "prompt": "closeup on the singer", + "references": [{ "reference_type": "…", "from_previous_result": "draw_singer" }] } +] +``` + +and downstream + +```json +{ "name": "edit", + "task": { "command": "concat_videos", "arguments": { "videos": "gather:shot" } } } +``` + +Before the run starts, the engine replaces the `for_each` step with one +ordinary step per entry, named `shot@wide_open`, `shot@closeup` — the +entry's `name`, or its index for an entry without one. Those are the names +the manifest, the job's events and the gallery show, and `@` is reserved +for them: a hand-written step name may not contain it. An entry's `name` +must be unique in its list and match `^[a-zA-Z_][a-zA-Z0-9_-]*$`. Give +entries names: the step cache keys on the member name, so a shot inserted +in the middle of a named list leaves every other shot cached, while an +indexed list shifts every later shot onto a different entry and regenerates +it. + +`item:field` is the whole value of that field, so an entry can carry +anything a step argument can — including a `references` list whose length +differs by shot, with `from_previous_result` and `asset:` strings inside +it. Nothing is interpolated: `"item:prompt"` is the field, `"shot: item:prompt"` +is a literal string. + +Two `for_each` steps over the *same* list are paired by name: inside +`shot@closeup`, a reference to another `for_each` step `slice` over the same +`shots` list resolves to `slice@closeup`. That is how a shot reads the audio +slice cut for it when slicing and generating are two steps. It is the one +pairing the engine has; `for_each` runs over exactly one list, and there is +no zip and no loop index. + +Limits: a list has at most 32 entries. `release_pipeline` on a `for_each` +step releases after the *last* member. Each entry is a full generation, so +quote `cost × len(list)` before running a list-driven workflow, and +`validate_workflow` with the `arguments` you will run with: it expands your +list, not the template's default, and reports a duplicate name or a missing +field at the entry's path. +``` + +- [ ] **Step 3: Mirror in `CLAUDE.md`** + +After the `prompt:` bullet in Type System (line 152-156), add: + +```markdown +- A step carrying `for_each` (a list, or `variable:` naming one) is expanded by + `expand_for_each` (`dw/for_each.py`) into one ordinary step per entry, named + `@`, immediately after `replace_variables` in + `Workflow.run` and, with the caller's arguments folded, in `validation_errors`. + Inside a member `item:` / `item:field` is the entry (any type, spliced whole); + a later step reads the group with `gather:` (a list; splices inside a + list); two groups over the same list pair by key (`slice` inside `shot@x` is + `slice@x`). `previous_result:` naming a group is a directed error. `@` is + reserved in step names; entry names are validated and unique; 32 entries max; + `release_pipeline`/`release_models` survive on the last member only. The + realized workflow keeps `for_each`; the manifest names the members +``` + +And in Critical Gotchas, after the `previous_result:` static-check bullet: + +```markdown +- **`for_each` expands before the reference check** — `validation_errors` substitutes + (the caller's `arguments` when they are all good, else the defaults) and expands + first, so `gather:` and `item:` errors carry the path of the template step + (`steps[0].for_each[1].name`) while a bad reference inside a member carries the + member's. Since substitution now precedes the check, a `from_previous_result` + spelled by a *declared* variable is checked by its value +``` + +- [ ] **Step 4: The manifest note in `docs/SERVER.md`** + +Find the manifest description (`grep -n "manifest" docs/SERVER.md`) and add one sentence where the manifest's step entries are described: + +```markdown +A `for_each` step appears in the manifest as its members (`shot@wide_open`, +`shot@closeup`), because the manifest records what ran; the run's +`workflow.json` keeps the `for_each` form, because it records what was asked. +``` + +- [ ] **Step 5: Run the guide tests** + +Run: `python -m pytest tests/test_server_guides.py tests/test_mcp_guides.py tests/test_plugin_skills.py -q` +Expected: PASS (the guide is served by heading; a new heading is a new section, nothing pins the count). + +- [ ] **Step 6: Commit** + +```bash +git add docs/WORKFLOW_GUIDE.md CLAUDE.md docs/SERVER.md +git commit -m "docs(for_each): the item:/gather:/for_each conventions for agents, CLAUDE.md mirror, manifest note + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 8: End-to-end on the server, then mark the proposal + +**Files:** +- Modify: `docs/proposals/list-driven-steps.md` (status line) +- Test: `tests/test_server_jobs.py` + +**Interfaces:** +- Consumes: everything above. + +- [ ] **Step 1: Write a job-level test** + +In `tests/test_server_jobs.py`, following the file's pattern for submitting an inline workflow and waiting for it (`grep -n "def test.*job\|/api/jobs" tests/test_server_jobs.py | head`), add a test that submits the `_for_each_workflow` from Task 5 (copy the dict - do not import from another test module) with `"arguments": {"shots": [{"name": "one", "text": "1"}, {"name": "two", "text": "2"}, {"name": "three", "text": "3"}]}` and asserts the finished job's manifest step names are `["shot@one", "shot@two", "shot@three", "edit"]` and that `edit`'s text output is `"123"` (or however `compose_text` joins - check `dw/tasks/` for its default separator and assert the actual value). + +- [ ] **Step 2: Run it** + +Run: `python -m pytest tests/test_server_jobs.py -k for_each -q` +Expected: PASS. If the worker path fails where the direct `Workflow.run` test passed, the difference is the spawned worker's own validation (`dw/worker.py:185`, `workflow.validate()` with no arguments) - that call validates against the defaults, which is fine, because the run itself then substitutes and expands the real arguments and any ForEachError fails the job with its message. + +- [ ] **Step 3: Full suite** + +Run: `python -m pytest tests -q` +Expected: PASS, no skips added. + +- [ ] **Step 4: Update the proposal status** + +In `docs/proposals/list-driven-steps.md`, change the status line to: + +```markdown +Status: **stage 1 implemented** (expansion pass, validation, schema, docs - +`dw/for_each.py`); stages 2 and 3 (templates, catalog cost and entry shape) +not started. Written for MCP feedback ticket T003. +``` + +- [ ] **Step 5: Commit** + +```bash +git add tests/test_server_jobs.py docs/proposals/list-driven-steps.md +git commit -m "test(for_each): a list-driven job end to end; proposal marks stage 1 done + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +## Self-review + +**Spec coverage:** expansion pass (T1), `item:` structured values (T1), same-key siblings (T2), `gather:` splice + errors (T2), directed errors (T2), `@` reserved + entry-name validation (T1), release flags on last member (T1), ceiling 32 (T1), schema (T4), validation ordering with folded arguments (T5, T6), realized workflow keeps `for_each` (no change needed - `realize_workflow` reads `self.workflow_definition`, which the pass never touches; stated in Global Constraints), manifest naming documented (T7), the `realize_args` check on entry objects (verified during planning: `resolve_path_references` in `dw/arguments.py:221` walks lists but leaves dicts alone, so an `asset:` inside an entry object stays a string until the member step realizes it - no exemption needed), templates expand back (T3), agent docs (T7). Cost/entry-shape in the catalog and the template rewrite are stages 2-3, deliberately out of this plan. + +**Placeholders:** Task 3's `dialogue-short` test asks the implementer to read the template and extend the entry - that is deliberate, because the entry shape is exactly what stage 2 has to get right and the template's argument differences are the ground truth; the assertion to converge on (`got[...] == expected`) is stated. Task 5 names the file's workflow-construction helper generically (`_workflow_from`) - use the helper `test_validate_catches_a_reference_to_a_step_that_does_not_exist` uses. + +**Type consistency:** `expand_for_each(definition) -> dict`, `ForEachError(path, message)` with `.path`, `member_name(group, key)`, `MAX_FOR_EACH_ENTRIES` are used identically across Tasks 1-6; `validation_errors(arguments=None)` is defined in Task 5 and called in Task 6. From 3b7e856ff53d368eb119d9d80a50b17053a09abe Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:21:04 -0500 Subject: [PATCH 02/38] feat(for_each): expansion pass - member naming, ceiling, item: substitution Co-Authored-By: Claude Fable 5.1 --- dw/for_each.py | 264 ++++++++++++++++++++++++++++++++++++++++ dw/previous_results.py | 16 +-- tests/test_for_each.py | 266 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 532 insertions(+), 14 deletions(-) create mode 100644 dw/for_each.py create mode 100644 tests/test_for_each.py diff --git a/dw/for_each.py b/dw/for_each.py new file mode 100644 index 00000000..5c554e36 --- /dev/null +++ b/dw/for_each.py @@ -0,0 +1,264 @@ +"""Expand a step's 'for_each' list into one ordinary step per entry. + +A template that generates one shot per entry of a list is otherwise written +the long way - 'shot_1', 'shot_2', ... each a near-copy of the one before - +and a six-shot episode is a different file from a five-shot one. This pass +runs on the definition after variable substitution and before the run id is +computed (Workflow.run) and before the reference check (validation_errors), +and produces a definition with no 'for_each' in it: every member is an +ordinary step, so the step loop, the step cache, the manifest and the +reference checker never learn a new reference kind. + +Inside a member: + - 'item:' is the whole entry; 'item:field' one field of an object + entry, spliced in whole whatever its type + - a reference to another for_each group over the SAME list resolves to + the member with the same key ('slice' inside 'shot@open' -> 'slice@open') +Outside (or inside, for any other group): + - 'gather:shot' is the list of every member's result, as explicit + 'previous_result:shot@' strings; inside a list it splices + - 'previous_result:shot' naming a group is an error that says to gather + +Members are named '@' - the entry's own 'name' when it carries +one, else its index - and '@' is reserved in every step name. Names rather +than indexes because the step cache (dw/step_cache.py) keys on the step +name: inserting a shot in the middle of a list must not shift every later +member onto a different entry's cache line. +""" + +import copy + +from .arguments import FROM_PREVIOUS_RESULT_KEY, PREVIOUS_RESULT_PREFIX +from .security import InvalidInputError, validate_variable_name +from .step_cache import reference_resolves_to + +FOR_EACH_KEY = "for_each" +ITEM_PREFIX = "item:" +GATHER_PREFIX = "gather:" +MEMBER_SEPARATOR = "@" +# Each entry is a full generation. Stated against the step cache's bound +# (DEFAULT_MAX_ENTRIES = 50): a run whose expanded steps exceed the cache +# evicts its own earlier members, so this is kept well under it +MAX_FOR_EACH_ENTRIES = 32 +# release_pipeline / release_models would drop the model after the first +# member and reload it for the second, so they are carried onto the last one +_LAST_MEMBER_ONLY = ("release_pipeline", "release_models") + + +class ForEachError(ValueError): + """A for_each step that cannot be expanded, with the JSON path at fault.""" + + def __init__(self, path, message): + super().__init__(message) + self.path = path + + +def member_name(group, key): + return f"{group}{MEMBER_SEPARATOR}{key}" + + +def expand_for_each(definition): + """The definition with every 'for_each' step replaced by its members. + + Returns a new structure; `definition` is left as it was passed in. + Raises ForEachError for anything that cannot be expanded. + """ + steps = definition.get("steps") if isinstance(definition, dict) else None + if not isinstance(steps, list): + return definition + + # group name -> {"keys": [...], "entries": [...]} for every group + # expanded so far, in step order, so a reference can only reach an + # earlier group - the same rule previous_result: has always had + groups = {} + expanded = [] + for index, step in enumerate(steps): + if not isinstance(step, dict): + expanded.append(copy.deepcopy(step)) + continue + path = ("steps", index) + name = step.get("name") + if isinstance(name, str) and MEMBER_SEPARATOR in name: + raise ForEachError( + render_path(path + ("name",)), + f"Step name '{name}' contains '{MEMBER_SEPARATOR}', which is " + f"reserved for the members of a for_each step", + ) + if FOR_EACH_KEY not in step: + expanded.append(_rewrite(step, path, groups, member=None)) + continue + + entries = step[FOR_EACH_KEY] + keys = _entry_keys(entries, path + (FOR_EACH_KEY,)) + template = {k: v for k, v in step.items() if k != FOR_EACH_KEY} + last = len(keys) - 1 + for position, (key, entry) in enumerate(zip(keys, entries)): + member = { + "group": name, + "key": key, + "entry": entry, + "entries": entries, + "index": position, + } + expanded_step = _rewrite(template, path, groups, member) + expanded_step["name"] = member_name(name, key) + if position != last: + for flag in _LAST_MEMBER_ONLY: + expanded_step.pop(flag, None) + expanded.append(expanded_step) + groups[name] = {"keys": keys, "entries": entries} + + result = {k: v for k, v in definition.items() if k != "steps"} + result["steps"] = expanded + return result + + +def _entry_keys(entries, path): + """The key of every entry - its 'name' when it is an object carrying + one, else its index - validated and unique.""" + if not isinstance(entries, list): + if isinstance(entries, str) and entries.startswith("variable:"): + hint = f" - '{entries}' was not substituted; is the variable declared?" + else: + hint = "" + raise ForEachError( + render_path(path), f"for_each must be a list, got {type(entries).__name__}{hint}" + ) + if len(entries) > MAX_FOR_EACH_ENTRIES: + raise ForEachError( + render_path(path), + f"for_each has {len(entries)} entries; the limit is {MAX_FOR_EACH_ENTRIES}", + ) + keys = [] + for index, entry in enumerate(entries): + key = str(index) + if isinstance(entry, dict) and "name" in entry: + key = entry["name"] + key_path = render_path(path + (index, "name")) + if not isinstance(key, str): + raise ForEachError(key_path, "An entry's name must be a string") + try: + validate_variable_name(key) + except InvalidInputError as e: + raise ForEachError(key_path, f"Invalid entry name '{key}': {e}") from e + if key in keys: + raise ForEachError(key_path, f"Duplicate entry name '{key}'") + keys.append(key) + return keys + + +def _rewrite(value, path, groups, member): + """Rebuild `value` with item:, gather: and group references resolved. + + `member` is None outside a for_each step; inside one it carries the + group, key and entry of the member being built. + """ + if isinstance(value, dict): + rebuilt = {} + for key, item in value.items(): + if key == FROM_PREVIOUS_RESULT_KEY and isinstance(item, str): + rebuilt[key] = _rewrite_reference(item, path + (key,), groups, member) + else: + rebuilt[key] = _rewrite(item, path + (key,), groups, member) + return rebuilt + if isinstance(value, list): + rebuilt = [] + for index, item in enumerate(value): + if isinstance(item, str) and item.startswith(GATHER_PREFIX): + # A gather inside a list splices into it + rebuilt.extend(_gather(item, path + (index,), groups)) + else: + rebuilt.append(_rewrite(item, path + (index,), groups, member)) + return rebuilt + if isinstance(value, str): + if value.startswith(GATHER_PREFIX): + return _gather(value, path, groups) + if value.startswith(ITEM_PREFIX): + return _item(value, path, member) + if value.startswith(PREVIOUS_RESULT_PREFIX): + reference = value[len(PREVIOUS_RESULT_PREFIX) :] + return PREVIOUS_RESULT_PREFIX + _rewrite_reference( + reference, path, groups, member + ) + return value + return copy.deepcopy(value) + + +def _item(value, path, member): + if member is None: + raise ForEachError( + render_path(path), f"'{value}' is only meaningful inside a for_each step" + ) + field = value[len(ITEM_PREFIX) :] + entry = member["entry"] + if field == "": + return copy.deepcopy(entry) + if not isinstance(entry, dict): + raise ForEachError( + render_path(path), + f"'{value}' asks for a field of entry '{member['key']}' of " + f"for_each step '{member['group']}', which is not an object", + ) + if field not in entry: + raise ForEachError( + render_path(path), + f"'{value}' names no field of entry '{member['key']}' of for_each " + f"step '{member['group']}'; it has: {sorted(entry)}", + ) + return copy.deepcopy(entry[field]) + + +def _gather(value, path, groups): + group = value[len(GATHER_PREFIX) :] + if group not in groups: + raise ForEachError( + render_path(path), + f"'{value}' names no earlier for_each step. " + f"for_each steps available here: {sorted(groups)}", + ) + return [ + PREVIOUS_RESULT_PREFIX + member_name(group, key) + for key in groups[group]["keys"] + ] + + +def _rewrite_reference(reference, path, groups, member): + """A previous_result reference (without its prefix) as the expanded + definition spells it: unchanged unless it names a for_each group.""" + if reference.startswith("variable:"): + return reference + group = next((g for g in groups if reference_resolves_to(reference, g)), None) + if group is None: + if member is not None and reference_resolves_to(reference, member["group"]): + raise ForEachError( + render_path(path), + f"'{reference}' names its own for_each step '{member['group']}'", + ) + return reference + if member is not None and member["entries"] == groups[group]["entries"]: + # Same list: shot@open reads slice@open + return member_name(group, member["key"]) + reference[len(group) :] + where = ( + f"from inside for_each step '{member['group']}', which runs over a different list" + if member is not None + else "from outside a for_each step" + ) + raise ForEachError( + render_path(path), + f"'{reference}' names the for_each step '{group}' {where}. Use " + f"'{GATHER_PREFIX}{group}' for every member's result, or a reference " + f"from a for_each step over the same list for the same-keyed member", + ) + + +def render_path(path): + """'steps[3].task.arguments.videos[1]' - the same shape schema errors use.""" + rendered = "" + for part in path: + if isinstance(part, int): + rendered += f"[{part}]" + elif rendered: + rendered += f".{part}" + else: + rendered = str(part) + return rendered diff --git a/dw/previous_results.py b/dw/previous_results.py index 14619298..3fa0a2e1 100644 --- a/dw/previous_results.py +++ b/dw/previous_results.py @@ -6,6 +6,7 @@ PREVIOUS_RESULT_PREFIX, build_objects, ) +from .for_each import render_path from .step_cache import reference_resolves_to logger = logging.getLogger("dw") @@ -335,7 +336,7 @@ def previous_result_reference_errors(workflow_definition): for path, reference in sorted(found.items(), key=lambda item: str(item[0])): if any(reference_resolves_to(reference, name) for name in seen): continue - location = _render_path(("steps", index) + path) + location = render_path(("steps", index) + path) errors.append( { "path": location, @@ -370,16 +371,3 @@ def _collect_reference_paths(value, path, found): _collect_reference_paths(item, path + (index,), found) elif isinstance(value, str) and value.startswith(PREVIOUS_RESULT_PREFIX): found[path] = value[len(PREVIOUS_RESULT_PREFIX) :] - - -def _render_path(path): - """'steps[3].task.arguments.videos[1]' - the same shape schema errors use.""" - rendered = "" - for part in path: - if isinstance(part, int): - rendered += f"[{part}]" - elif rendered: - rendered += f".{part}" - else: - rendered = str(part) - return rendered diff --git a/tests/test_for_each.py b/tests/test_for_each.py new file mode 100644 index 00000000..9e8e81b1 --- /dev/null +++ b/tests/test_for_each.py @@ -0,0 +1,266 @@ +import copy + +import pytest + +from dw.for_each import ( + MAX_FOR_EACH_ENTRIES, + ForEachError, + expand_for_each, + member_name, +) + + +def definition(*steps, **extra): + return {"id": "test", "steps": list(steps), **extra} + + +class TestNaming: + def test_member_name_joins_with_at(self): + assert member_name("shot", "wide_open") == "shot@wide_open" + + def test_an_object_entry_is_named_by_its_name(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "wide_open"}, {"name": "closeup"}], + "task": {"command": "x", "arguments": {}}, + } + ) + ) + assert [s["name"] for s in expanded["steps"]] == [ + "shot@wide_open", + "shot@closeup", + ] + + def test_a_nameless_entry_is_named_by_its_index(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": ["a", "b"], "task": {"arguments": {}}} + ) + ) + assert [s["name"] for s in expanded["steps"]] == ["shot@0", "shot@1"] + + def test_for_each_is_dropped_from_the_members(self): + expanded = expand_for_each( + definition({"name": "shot", "for_each": ["a"], "task": {}}) + ) + assert "for_each" not in expanded["steps"][0] + + def test_the_input_is_not_mutated(self): + original = definition({"name": "shot", "for_each": ["a"], "task": {}}) + before = copy.deepcopy(original) + expand_for_each(original) + assert original == before + + def test_a_workflow_without_for_each_comes_back_equal(self): + original = definition({"name": "plain", "task": {"arguments": {"a": 1}}}) + assert expand_for_each(original) == original + + def test_a_hand_written_at_sign_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each(definition({"name": "shot@0", "task": {}})) + assert e.value.path == "steps[0].name" + assert "@" in str(e.value) + + def test_a_duplicate_entry_name_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "closeup"}, {"name": "closeup"}], + "task": {}, + } + ) + ) + assert e.value.path == "steps[0].for_each[1].name" + assert "closeup" in str(e.value) + + def test_an_invalid_entry_name_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": [{"name": "wide open"}], "task": {}} + ) + ) + assert e.value.path == "steps[0].for_each[0].name" + + def test_a_non_list_for_each_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each(definition({"name": "shot", "for_each": 4, "task": {}})) + assert e.value.path == "steps[0].for_each" + assert "list" in str(e.value) + + def test_an_unsubstituted_variable_is_an_error_that_says_so(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "shot", "for_each": "variable:shots", "task": {}}) + ) + assert "variable:shots" in str(e.value) + + def test_an_empty_list_expands_to_no_steps(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": [], "task": {}}, + {"name": "after", "task": {}}, + ) + ) + assert [s["name"] for s in expanded["steps"]] == ["after"] + + def test_the_ceiling_is_enforced(self): + entries = [{"name": f"s{i}"} for i in range(MAX_FOR_EACH_ENTRIES + 1)] + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "shot", "for_each": entries, "task": {}}) + ) + assert str(MAX_FOR_EACH_ENTRIES) in str(e.value) + + def test_a_list_at_the_ceiling_is_fine(self): + entries = [{"name": f"s{i}"} for i in range(MAX_FOR_EACH_ENTRIES)] + expanded = expand_for_each( + definition({"name": "shot", "for_each": entries, "task": {}}) + ) + assert len(expanded["steps"]) == MAX_FOR_EACH_ENTRIES + + +class TestItemSubstitution: + def test_a_field_is_substituted(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a", "prompt": "the band walks on"}], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + assert expanded["steps"][0]["pipeline"]["arguments"]["prompt"] == ( + "the band walks on" + ) + + def test_a_bare_item_is_the_whole_entry(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": ["first prompt", "second prompt"], + "pipeline": {"arguments": {"prompt": "item:"}}, + } + ) + ) + prompts = [s["pipeline"]["arguments"]["prompt"] for s in expanded["steps"]] + assert prompts == ["first prompt", "second prompt"] + + def test_a_structured_field_is_spliced_whole(self): + references = [ + {"reference_type": "T", "from_previous_result": "draw_a"}, + {"reference_type": "T", "from_previous_result": "draw_b"}, + ] + expanded = expand_for_each( + definition( + {"name": "draw_a", "task": {}}, + {"name": "draw_b", "task": {}}, + { + "name": "shot", + "for_each": [{"name": "open", "references": references}], + "pipeline": {"arguments": {"references": "item:references"}}, + }, + ) + ) + assert expanded["steps"][2]["pipeline"]["arguments"]["references"] == references + + def test_a_number_keeps_its_type(self): + expanded = expand_for_each( + definition( + { + "name": "slice", + "for_each": [{"name": "a", "start_frame": 124}], + "task": {"arguments": {"start_frame": "item:start_frame"}}, + } + ) + ) + assert expanded["steps"][0]["task"]["arguments"]["start_frame"] == 124 + + def test_item_is_substituted_at_any_depth(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a", "voice": "asset:a.wav"}], + "pipeline": { + "arguments": {"references": [{"from_file": "item:voice"}]} + }, + } + ) + ) + assert expanded["steps"][0]["pipeline"]["arguments"]["references"] == [ + {"from_file": "asset:a.wav"} + ] + + def test_a_missing_field_is_an_error_at_its_path(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + assert e.value.path == "steps[0].pipeline.arguments.prompt" + assert "prompt" in str(e.value) and "a" in str(e.value) + + def test_a_field_of_a_string_entry_is_an_error(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + { + "name": "shot", + "for_each": ["just a prompt"], + "pipeline": {"arguments": {"prompt": "item:prompt"}}, + } + ) + ) + + def test_item_outside_a_for_each_step_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition({"name": "plain", "task": {"arguments": {"p": "item:x"}}}) + ) + assert e.value.path == "steps[0].task.arguments.p" + + def test_members_do_not_share_mutable_values(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}, {"name": "b"}], + "pipeline": {"arguments": {"references": [{"k": 1}]}}, + } + ) + ) + first, second = expanded["steps"] + first["pipeline"]["arguments"]["references"][0]["k"] = 2 + assert second["pipeline"]["arguments"]["references"][0]["k"] == 1 + + +class TestRelease: + def test_release_flags_survive_on_the_last_member_only(self): + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": [{"name": "a"}, {"name": "b"}, {"name": "c"}], + "release_pipeline": True, + "release_models": True, + "pipeline": {}, + } + ) + ) + flags = [ + (s.get("release_pipeline"), s.get("release_models")) + for s in expanded["steps"] + ] + assert flags == [(None, None), (None, None), (True, True)] From 2a2ea708404e45d53f18eb654c8e5093b880b924 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:28:02 -0500 Subject: [PATCH 03/38] test(for_each): gather:, same-key siblings and the directed group errors Co-Authored-By: Claude Fable 5.1 --- tests/test_for_each.py | 255 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 255 insertions(+) diff --git a/tests/test_for_each.py b/tests/test_for_each.py index 9e8e81b1..e8e26292 100644 --- a/tests/test_for_each.py +++ b/tests/test_for_each.py @@ -264,3 +264,258 @@ def test_release_flags_survive_on_the_last_member_only(self): for s in expanded["steps"] ] assert flags == [(None, None), (None, None), (True, True)] + + +class TestGather: + def group(self, *extra): + return definition( + { + "name": "shot", + "for_each": [{"name": "open"}, {"name": "close"}], + "pipeline": {"arguments": {}}, + }, + *extra, + ) + + def test_a_scalar_gather_becomes_the_member_list(self): + expanded = expand_for_each( + self.group( + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}} + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [ + "previous_result:shot@open", + "previous_result:shot@close", + ] + + def test_a_gather_inside_a_list_splices(self): + expanded = expand_for_each( + self.group( + {"name": "intro", "task": {}}, + { + "name": "edit", + "task": { + "arguments": { + "videos": ["previous_result:intro", "gather:shot"] + } + }, + }, + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [ + "previous_result:intro", + "previous_result:shot@open", + "previous_result:shot@close", + ] + + def test_gather_of_an_empty_group_is_an_empty_list(self): + expanded = expand_for_each( + definition( + {"name": "shot", "for_each": [], "task": {}}, + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}}, + ) + ) + assert expanded["steps"][-1]["task"]["arguments"]["videos"] == [] + + def test_gather_of_a_plain_step_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "one", "task": {}}, + {"name": "edit", "task": {"arguments": {"videos": "gather:one"}}}, + ) + ) + assert e.value.path == "steps[1].task.arguments.videos" + assert "no earlier for_each step" in str(e.value) + + def test_gather_of_a_later_group_is_an_error(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + {"name": "edit", "task": {"arguments": {"videos": "gather:shot"}}}, + {"name": "shot", "for_each": ["a"], "task": {}}, + ) + ) + + def test_gather_in_a_sub_workflow_argument_map(self): + expanded = expand_for_each( + self.group( + { + "name": "score", + "workflow": {"path": "builtin:x.json", "arguments": {"clips": "gather:shot"}}, + } + ) + ) + assert expanded["steps"][-1]["workflow"]["arguments"]["clips"] == [ + "previous_result:shot@open", + "previous_result:shot@close", + ] + + +class TestGroupReferences: + def test_previous_result_naming_a_group_says_to_gather(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": {"arguments": {"video": "previous_result:shot"}}, + }, + ) + ) + assert e.value.path == "steps[1].task.arguments.video" + assert "gather:shot" in str(e.value) + + def test_from_previous_result_naming_a_group_says_to_gather(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": { + "arguments": {"refs": [{"from_previous_result": "shot"}]} + }, + }, + ) + ) + assert e.value.path == "steps[1].task.arguments.refs[0].from_previous_result" + + def test_a_property_reference_to_a_group_is_also_refused(self): + with pytest.raises(ForEachError): + expand_for_each( + definition( + {"name": "shot", "for_each": ["a"], "task": {}}, + { + "name": "edit", + "task": {"arguments": {"v": "previous_result:shot.frames"}}, + }, + ) + ) + + def test_a_reference_to_an_ordinary_step_is_untouched(self): + expanded = expand_for_each( + definition( + {"name": "draw", "task": {}}, + { + "name": "shot", + "for_each": ["a"], + "pipeline": { + "arguments": { + "references": [{"from_previous_result": "draw"}], + "still": "previous_result:draw.image", + } + }, + }, + ) + ) + arguments = expanded["steps"][1]["pipeline"]["arguments"] + assert arguments["references"] == [{"from_previous_result": "draw"}] + assert arguments["still"] == "previous_result:draw.image" + + def test_a_variable_spelled_from_previous_result_is_untouched(self): + expanded = expand_for_each( + definition( + { + "name": "edit", + "task": {"arguments": {"r": [{"from_previous_result": "variable:x"}]}}, + } + ) + ) + assert expanded["steps"][0]["task"]["arguments"]["r"] == [ + {"from_previous_result": "variable:x"} + ] + + +class TestSameKeySiblings: + def shots(self): + return [ + {"name": "open", "start_frame": 0}, + {"name": "close", "start_frame": 124}, + ] + + def test_a_sibling_over_the_same_list_resolves_to_the_same_key(self): + shots = self.shots() + expanded = expand_for_each( + definition( + { + "name": "slice", + "for_each": shots, + "task": {"arguments": {"start_frame": "item:start_frame"}}, + }, + { + "name": "shot", + "for_each": shots, + "pipeline": { + "arguments": { + "references": [{"from_previous_result": "slice"}], + "audio": "previous_result:slice.audio", + } + }, + }, + ) + ) + names = [s["name"] for s in expanded["steps"]] + assert names == ["slice@open", "slice@close", "shot@open", "shot@close"] + close = expanded["steps"][3]["pipeline"]["arguments"] + assert close["references"] == [{"from_previous_result": "slice@close"}] + assert close["audio"] == "previous_result:slice@close.audio" + + def test_the_same_list_means_equal_not_identical(self): + expanded = expand_for_each( + definition( + {"name": "slice", "for_each": self.shots(), "task": {}}, + { + "name": "shot", + "for_each": self.shots(), + "task": {"arguments": {"a": "previous_result:slice"}}, + }, + ) + ) + assert expanded["steps"][2]["task"]["arguments"]["a"] == ( + "previous_result:slice@open" + ) + + def test_a_sibling_over_a_different_list_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + {"name": "slice", "for_each": ["a", "b"], "task": {}}, + { + "name": "shot", + "for_each": ["a"], + "task": {"arguments": {"x": "previous_result:slice"}}, + }, + ) + ) + assert "different list" in str(e.value) + + def test_a_step_referencing_its_own_group_is_an_error(self): + with pytest.raises(ForEachError) as e: + expand_for_each( + definition( + { + "name": "shot", + "for_each": ["a", "b"], + "task": {"arguments": {"x": "previous_result:shot"}}, + } + ) + ) + assert "its own" in str(e.value) + + def test_a_gather_inside_a_member_still_gathers(self): + expanded = expand_for_each( + definition( + {"name": "slice", "for_each": ["a", "b"], "task": {}}, + { + "name": "shot", + "for_each": ["x"], + "task": {"arguments": {"all": "gather:slice"}}, + }, + ) + ) + assert expanded["steps"][2]["task"]["arguments"]["all"] == [ + "previous_result:slice@0", + "previous_result:slice@1", + ] From 60358b3f6205fe126772ba783a16c2f470dad792 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:34:11 -0500 Subject: [PATCH 04/38] test(for_each): music-video and dialogue-short expand back to their hand-written steps Co-Authored-By: Claude Fable 5.1 --- tests/test_for_each.py | 141 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/tests/test_for_each.py b/tests/test_for_each.py index e8e26292..813782dd 100644 --- a/tests/test_for_each.py +++ b/tests/test_for_each.py @@ -1,4 +1,6 @@ import copy +import json +import os import pytest @@ -14,6 +16,31 @@ def definition(*steps, **extra): return {"id": "test", "steps": list(steps), **extra} +TEMPLATES = os.path.join( + os.path.dirname(__file__), "..", "workflows", "templates", "minimax" +) + + +def load_template(name): + with open(os.path.join(TEMPLATES, name)) as f: + return json.load(f) + + +def steps_by_name(definition): + return {s["name"]: s for s in definition["steps"]} + + +def without_pipeline_reference(step, pipeline_step): + """A hand-written 'pipeline_reference' shot as the full pipeline block + the expansion produces: the reference's arguments over the referenced + step's pipeline.""" + rebuilt = {k: v for k, v in step.items() if k != "pipeline_reference"} + pipeline = copy.deepcopy(pipeline_step["pipeline"]) + pipeline["arguments"] = step["pipeline_reference"]["arguments"] + rebuilt["pipeline"] = pipeline + return rebuilt + + class TestNaming: def test_member_name_joins_with_at(self): assert member_name("shot", "wide_open") == "shot@wide_open" @@ -519,3 +546,117 @@ def test_a_gather_inside_a_member_still_gathers(self): "previous_result:slice@0", "previous_result:slice@1", ] + + +class TestMusicVideoTemplate: + """music-video's four slices and four shots, written as two for_each + groups over one 'shots' list, expand to the steps the template holds + by hand today.""" + + def test_the_hand_written_shots_are_what_the_list_expands_to(self): + template = load_template("music-video.json") + today = steps_by_name(template) + shots = [ + {"name": "wide_open", "prompt": "variable:shot_1_wide_open", "start_frame": 0}, + {"name": "closeup", "prompt": "variable:shot_2_closeup", "start_frame": 124}, + {"name": "room", "prompt": "variable:shot_3_room", "start_frame": 248}, + {"name": "finale", "prompt": "variable:shot_4_finale", "start_frame": 372}, + ] + slice_template = copy.deepcopy(today["slice_1"]) + slice_template["name"] = "slice" + slice_template["for_each"] = shots + slice_template["task"]["arguments"]["start_frame"] = "item:start_frame" + + shot_template = copy.deepcopy(today["shot_1_wide_open"]) + shot_template["name"] = "shot" + shot_template["for_each"] = shots + shot_template["pipeline"]["arguments"]["prompt"] = "item:prompt" + for reference in shot_template["pipeline"]["arguments"]["references"]: + if reference.get("from_previous_result") == "slice_1": + reference["from_previous_result"] = "slice" + + edit = copy.deepcopy(today["edit"]) + edit["task"]["arguments"]["videos"] = "gather:shot" + + expanded = expand_for_each( + definition( + today["draw_singer"], today["write_song"], slice_template, + today["soundtrack"], shot_template, edit, today["music_video"], + ) + ) + got = steps_by_name(expanded) + + # Each expanded slice is today's slice with the new name + for key, old in zip(["wide_open", "closeup", "room", "finale"], range(1, 5)): + expected = copy.deepcopy(today[f"slice_{old}"]) + expected["name"] = f"slice@{key}" + assert got[f"slice@{key}"] == expected + + # Each expanded shot is today's shot (as a full pipeline block) with + # the new name and its slice renamed + hand_written = ["shot_1_wide_open", "shot_2_closeup", "shot_3_room", "shot_4_finale"] + for key, old, index in zip(["wide_open", "closeup", "room", "finale"], hand_written, range(1, 5)): + step = today[old] + if "pipeline_reference" in step: + step = without_pipeline_reference(step, today["shot_1_wide_open"]) + expected = copy.deepcopy(step) + expected["name"] = f"shot@{key}" + for reference in expected["pipeline"]["arguments"]["references"]: + if reference.get("from_previous_result") == f"slice_{index}": + reference["from_previous_result"] = f"slice@{key}" + assert got[f"shot@{key}"] == expected + + assert got["edit"]["task"]["arguments"]["videos"] == [ + f"previous_result:shot@{k}" for k in ["wide_open", "closeup", "room", "finale"] + ] + + +class TestDialogueShortTemplate: + """dialogue-short's five shots, whose reference lists and frame counts + differ by shot, written as one for_each group whose entries carry every + argument that differs between shots.""" + + def test_the_hand_written_shots_are_what_the_list_expands_to(self): + template = load_template("dialogue-short.json") + today = steps_by_name(template) + hand_written = [ + ("cold_open", "shot_1_cold_open"), + ("deflect", "shot_2_deflect"), + ("react", "shot_3_react"), + ("button", "shot_4_button"), + ("tag", "shot_5_tag"), + ] + first = today["shot_1_cold_open"] + full = { + old: (step if "pipeline_reference" not in step else without_pipeline_reference(step, first)) + for old, step in today.items() + if old.startswith("shot_") + } + + shots = [] + for key, old in hand_written: + arguments = full[old]["pipeline"]["arguments"] + shots.append( + { + "name": key, + "prompt": arguments["prompt"], + "references": arguments["references"], + "num_frames": arguments["num_frames"], + } + ) + + shot_template = copy.deepcopy(full["shot_1_cold_open"]) + shot_template["name"] = "shot" + shot_template["for_each"] = shots + shot_template["pipeline"]["arguments"]["prompt"] = "item:prompt" + shot_template["pipeline"]["arguments"]["references"] = "item:references" + shot_template["pipeline"]["arguments"]["num_frames"] = "item:num_frames" + + expanded = expand_for_each( + definition(today["draw_character_a"], today["draw_character_b"], shot_template) + ) + got = steps_by_name(expanded) + for key, old in hand_written: + expected = copy.deepcopy(full[old]) + expected["name"] = f"shot@{key}" + assert got[f"shot@{key}"] == expected From d4ca7a14f2f029f6d708210d42de31c5d0103200 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:39:25 -0500 Subject: [PATCH 05/38] feat(schema): for_each on a step Co-Authored-By: Claude Fable 5.1 --- dw/workflow_schema.json | 6 ++++++ tests/test_schema.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/dw/workflow_schema.json b/dw/workflow_schema.json index 7fe65852..f3f160cc 100644 --- a/dw/workflow_schema.json +++ b/dw/workflow_schema.json @@ -104,6 +104,12 @@ "name": { "type": "string" }, + "for_each": { + "description": "Run this step once per entry of a list. A list, or a 'variable:' reference to one. Inside the step, 'item:' is the entry and 'item:field' one of its fields; a later step reads every member's result with 'gather:'. Members are named '@' (or '@' for an entry without a name), so '@' is reserved in step names.", + "type": ["array", "string"], + "pattern": "^variable:", + "maxItems": 32 + }, "seed": { "description": "Default seed for the entire step. Accepts a 'variable:' reference.", "type": [ diff --git a/tests/test_schema.py b/tests/test_schema.py index 7a2df3e3..8727d26d 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -394,3 +394,23 @@ def test_a_capped_list_says_so(self): text = format_validation_errors(errors) assert text.startswith(f"Validation errors (first {MAX_VALIDATION_ERRORS}):\n") + + +def test_for_each_is_a_list_or_a_variable_reference(): + schema = load_schema("workflow") + base = { + "id": "t", + "steps": [ + { + "name": "shot", + "task": {"command": "x", "arguments": {}}, + "for_each": None, + } + ], + } + for good in (["a", "b"], [{"name": "a"}], "variable:shots"): + base["steps"][0]["for_each"] = good + assert validate_data_all(base, schema) == [] + for bad in (4, "shots", {"a": 1}): + base["steps"][0]["for_each"] = bad + assert validate_data_all(base, schema) != [] From 83e5bec32abbd364a1ceef5b44496b950b119a42 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:46:37 -0500 Subject: [PATCH 06/38] feat(for_each): expand in Workflow.run and validate the expanded definition Co-Authored-By: Claude Fable 5.1 --- dw/workflow.py | 48 +++++++++++++++++++++++--- tests/test_workflow.py | 78 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 5 deletions(-) diff --git a/dw/workflow.py b/dw/workflow.py index cea1b086..d0d3ce63 100644 --- a/dw/workflow.py +++ b/dw/workflow.py @@ -42,7 +42,13 @@ ) from .realize import realize_workflow from .schema import validate_data_all, format_validation_errors, load_schema -from .variables import replace_variables, set_variables +from .for_each import expand_for_each, ForEachError +from .variables import ( + argument_errors, + replace_variables, + set_variables, + VariableNotFoundError, +) from .pipeline_processors.pipeline import Pipeline from .tasks.model_cache import clear_model_cache from .tasks.task import Task @@ -282,16 +288,42 @@ def effective_output_dir(self): os.path.join(self.output_dir, subfolder) if subfolder else self.output_dir ) - def validation_errors(self): + def expanded_definition(self, arguments=None): + """The definition as the run will see it: variables substituted - + the caller's `arguments` folded in when they are all good, else the + declared defaults - and every for_each step expanded. + + Raises ForEachError for a for_each that cannot be expanded. A + 'variable:' that names nothing is left in place rather than raised: + validate_workflow already reports that as a warning, and the + reference check is happy to skip a reference it cannot read. + """ + definition = copy.deepcopy(self.workflow_definition) + variables = definition.get("variables") + if isinstance(variables, dict): + if arguments and not argument_errors(definition, arguments): + set_variables(arguments, variables) + try: + definition = replace_variables(definition, variables) + except VariableNotFoundError: + pass + return expand_for_each(definition) + + def validation_errors(self, arguments=None): """Every schema violation in the definition, as [{path, message}]; - empty when it validates.""" + empty when it validates. `arguments` are the caller's, so a + for_each over a list the caller supplies is checked as it will run.""" errors = validate_data_all(self.workflow_definition, load_schema("workflow")) - # Only once the shape is known good: the reference pass walks the + # Only once the shape is known good: the passes below walk the # steps array and a definition that fails the schema may have no # such array to walk if errors: return errors - return previous_result_reference_errors(self.workflow_definition) + try: + expanded = self.expanded_definition(arguments) + except ForEachError as e: + return [{"path": e.path, "message": str(e)}] + return previous_result_reference_errors(expanded) def validate(self): """Validates workflow definition against JSON schema. @@ -385,6 +417,12 @@ def run( # place, so the result must be captured here workflow_def = replace_variables(workflow_def, variables) + # One ordinary step per entry of every for_each list, before the + # seed, the run id and the realized workflow are computed, so + # each covers what actually runs. A ForEachError here fails the + # run before anything loads + workflow_def = expand_for_each(workflow_def) + # Set up random seed for reproducibility. Resolved lazily - as a # dict.get default, torch.seed() would run on every call and reseed # the global RNG even when the workflow names an explicit seed diff --git a/tests/test_workflow.py b/tests/test_workflow.py index bc973e90..2241bb02 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -613,3 +613,81 @@ def test_validate_catches_a_reference_to_a_step_that_does_not_exist(tmp_path): assert "steps[1].task.arguments.videos[1]" in message assert "shot_2_pat_deflects" in message assert message.count("Validation error") == 1 + + +def _workflow_from(definition, tmp_path): + return Workflow(definition, str(tmp_path), "") + + +def _for_each_workflow(**overrides): + definition = { + "id": "fe", + "variables": {"shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}]}, + "steps": [ + { + "name": "shot", + "for_each": "variable:shots", + "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "result": {"content_type": "text/plain"}, + }, + { + "name": "edit", + "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "result": {"content_type": "text/plain"}, + }, + ], + } + definition.update(overrides) + return definition + + +def test_validation_expands_for_each_before_the_reference_check(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) # the file's helper + assert workflow.validation_errors() == [] + + +def test_validation_reports_a_for_each_error_at_its_path(tmp_path): + definition = _for_each_workflow() + definition["steps"][1]["task"]["arguments"]["parts"] = "previous_result:shot" + workflow = _workflow_from(definition, tmp_path) + errors = workflow.validation_errors() + assert len(errors) == 1 + assert errors[0]["path"] == "steps[1].task.arguments.parts" + assert "gather:shot" in errors[0]["message"] + + +def test_validation_expands_the_callers_list_not_the_default(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + # Two entries share a name only in the caller's list + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a"}, {"name": "a"}]} + ) + assert errors and "Duplicate entry name 'a'" in errors[0]["message"] + + +def test_validation_falls_back_to_the_default_when_the_arguments_are_bad(tmp_path): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + # An undeclared argument is argument_errors' finding, not validation's + assert workflow.validation_errors(arguments={"nope": 1}) == [] + + +def test_validation_of_an_undeclared_variable_still_does_not_raise(tmp_path): + definition = _for_each_workflow() + definition["steps"][0]["for_each"] = "variable:missing" + workflow = _workflow_from(definition, tmp_path) + errors = workflow.validation_errors() + assert errors and "variable:missing" in errors[0]["message"] + + +def test_run_expands_for_each_and_names_the_members(tmp_path): + workflow = _workflow_from(_for_each_workflow(seed=1), tmp_path) + workflow.run({}) + names = [entry["step"] for entry in workflow.manifest] + assert names == ["shot@a", "shot@b", "edit"] + + +def test_run_substitutes_the_callers_list(tmp_path): + workflow = _workflow_from(_for_each_workflow(seed=1), tmp_path) + workflow.run({"shots": [{"name": "only", "text": "X"}]}) + names = [entry["step"] for entry in workflow.manifest] + assert names == ["shot@only", "edit"] From 2cc659b9fefa9df746db9af9943d15b5c5d8eafd Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:50:38 -0500 Subject: [PATCH 07/38] feat(server): POST /api/validate expands for_each over the caller's list Co-Authored-By: Claude Fable 5.1 --- dw/server/app.py | 4 +++- tests/test_validate_arguments.py | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/dw/server/app.py b/dw/server/app.py index d66a42ad..256d94ea 100644 --- a/dw/server/app.py +++ b/dw/server/app.py @@ -1267,7 +1267,9 @@ def validate_workflow( "has the detail", ) try: - errors = candidate.validation_errors() + # The caller's list is the one a for_each expands over, so the + # pre-flight checks the step set that will actually run + errors = candidate.validation_errors(arguments=request.arguments) except Exception: # An error here is not the schema's verdict on the workflow - # validation_errors() reports that by returning it. It is the diff --git a/tests/test_validate_arguments.py b/tests/test_validate_arguments.py index 5b4cefd6..3eb99f28 100644 --- a/tests/test_validate_arguments.py +++ b/tests/test_validate_arguments.py @@ -162,6 +162,39 @@ def test_arguments_are_checked_on_an_inline_definition_too(self, server): assert result["valid"] is False assert result["errors"][0]["path"] == "arguments.nope" + def test_validate_expands_for_each_with_the_callers_list(self, server): + workflow = { + "id": "fe", + "variables": {"shots": [{"name": "a", "text": "A"}]}, + "steps": [ + { + "name": "shot", + "for_each": "variable:shots", + "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "result": {"content_type": "text/plain"}, + }, + { + "name": "edit", + "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "result": {"content_type": "text/plain"}, + }, + ], + } + with server() as client: + ok = client.post("/api/validate", json={"workflow": workflow}).json() + assert ok["valid"] is True + + bad = client.post( + "/api/validate", + json={ + "workflow": workflow, + "arguments": {"shots": [{"name": "x"}, {"name": "x"}]}, + }, + ).json() + + assert bad["valid"] is False + assert bad["errors"][0]["path"] == "steps[0].for_each[1].name" + class TestSubmission: def test_a_bad_argument_is_refused_before_the_job_is_queued(self, server): From 171c0b73ff518f782b3495135d91b4de30f8c6cf Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 17:55:01 -0500 Subject: [PATCH 08/38] docs(for_each): the item:/gather:/for_each conventions for agents, CLAUDE.md mirror, manifest note Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 17 ++++++++++ docs/SERVER.md | 2 +- docs/WORKFLOW_GUIDE.md | 76 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 64084f75..c0b7ebff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -154,6 +154,17 @@ docs/WORKSPACES.md, and docs/proposals/server-workspaces.md for the later stages rooted at the library rather than the workflow file. The library is `DW_PROMPT_DIR` / `--prompt-dir`, else `./prompts` if it exists, else found by walking up from the workflow file's directory +- A step carrying `for_each` (a list, or `variable:` naming one) is expanded by + `expand_for_each` (`dw/for_each.py`) into one ordinary step per entry, named + `@`, immediately after `replace_variables` in + `Workflow.run` and, with the caller's arguments folded, in `validation_errors`. + Inside a member `item:` / `item:field` is the entry (any type, spliced whole); + a later step reads the group with `gather:` (a list; splices inside a + list); two groups over the same list pair by key (`slice` inside `shot@x` is + `slice@x`). `previous_result:` naming a group is a directed error. `@` is + reserved in step names; entry names are validated and unique; 32 entries max; + `release_pipeline`/`release_models` survive on the last member only. The + realized workflow keeps `for_each`; the manifest names the members - Every run directory holds `workflow.json` beside its manifest: the *realized* workflow, with the run's arguments folded into the variable defaults, the seed it used, stored prompt text inlined and `output:.../latest/...` pinned to the @@ -221,6 +232,12 @@ same reason - default setup cannot load a pack. renamed in one place and not another failed only when the run reached it, after every step before it had generated. A reference spelled by a `variable:` is left alone - what it names is not knowable before substitution +- **`for_each` expands before the reference check** — `validation_errors` substitutes + (the caller's `arguments` when they are all good, else the defaults) and expands + first, so `gather:` and `item:` errors carry the path of the template step + (`steps[0].for_each[1].name`) while a bad reference inside a member carries the + member's. Since substitution now precedes the check, a `from_previous_result` + spelled by a *declared* variable is checked by its value - **Cartesian product explosion** — multiple `previous_result` references multiply: 4 images × 3 masks = 12 iterations - **Component sharing requires exact key matching** between `shared_components` and `reused_components` - **Built-in workflows** need explicit argument mapping: `"prompt": "variable:prompt"` diff --git a/docs/SERVER.md b/docs/SERVER.md index ea5bd175..e9097f2b 100644 --- a/docs/SERVER.md +++ b/docs/SERVER.md @@ -138,7 +138,7 @@ from another machine: | --- | --- | | `POST /api/jobs` | Queue a run: `{"workflow_path": ...}` or an inline `{"workflow": {...}, "base_dir": ...}`, plus `arguments` for variable overrides. `workflow_path` accepts a stored workflow name as listed by `/api/workflows` (with or without `.json`, nested names included), or a relative/absolute path that still resolves under `--workflow-dir` - confined the same way the `/api/workflows` CRUD routes are; a path that names a real file outside that directory is rejected with 400, not opened. Answers with argument warnings from signature checking. | | `GET /api/jobs?workspace=&status=&limit=` | Queue + history summaries, oldest first, with `total` beside them. `status` narrows to one state or a comma-separated set (`queued`, `running`, `succeeded`, `failed`, `cancelled`; anything else is a 400); `limit` keeps the newest N, and `total` still reports how many matched, so a bounded answer cannot be mistaken for a complete one. No parameters means every job, which is what the web UI polls | -| `GET /api/jobs/{id}` | Full detail: spec, events, manifest, error. A manifest entry for a step served from the step cache carries `reused: true` | +| `GET /api/jobs/{id}` | Full detail: spec, events, manifest, error. A manifest entry for a step served from the step cache carries `reused: true`. A `for_each` step appears in the manifest as its members (`shot@wide_open`, `shot@closeup`), because the manifest records what ran; the run's `workflow.json` keeps the `for_each` form, because it records what was asked | | `GET /api/jobs/{id}/workflow` | The workflow the job ran: `{id, definition, realized, seed_variable}`. `seed_variable` names the variable a `new_seed` rerun would draw into (null when the workflow has none), read from the workflow as written rather than the realized copy, whose seed is pinned. `realized: true` is the copy the run itself wrote (`workflow.json` in its run directory), with arguments, seed, prompts and `output:latest` pinned; `false` falls back to the submitted definition, which is what a job from before run tracking has. 404 means neither is readable - the job itself still is | | `POST /api/jobs/{id}/export?workspace=&overwrite=` | Gather one finished job into `/exports//`: `workflow.json`, `manifest.json`, `job.json`, `README.md`, `assets/`, `inputs/`, `outputs/`. 201 with the file list, total bytes, anything it could not find, a `zip_url`, and the three JSON files inline. 404 unknown job, 409 for a job still running or an existing export without `overwrite` | | `GET /exports/{id}.zip?workspace=` | The same tree as one archive, built on request rather than kept as a second copy. Entries are named `/`. Ungated exactly as `/outputs` is | diff --git a/docs/WORKFLOW_GUIDE.md b/docs/WORKFLOW_GUIDE.md index 460a6de4..de294df5 100644 --- a/docs/WORKFLOW_GUIDE.md +++ b/docs/WORKFLOW_GUIDE.md @@ -271,6 +271,14 @@ not validation. - `prompt:` — `prompt:name` or `prompt:folder/name` is a stored prompt's `text`, rooted at the prompt library. That text may not itself begin with any of these prefixes; the engine rejects such a prompt rather than resolving twice. +- `item:` — only inside a step that carries `for_each`: `item:` is the + entry the member was made for, `item:field` one field of an object entry, + spliced in whole whatever its type — a string, a number, a list of + references. See "One step per entry" below. +- `gather:` — `gather:shot` is the result of *every* member of the + `for_each` step `shot`, in list order, as one list. Inside a list it splices + into it. It is how a step downstream of a fan-out reads the whole group; + `previous_result:shot` naming a `for_each` step is an error that says so. After a long inline run that is worth keeping, `get_job_workflow(job_id)` returns the realized workflow — the definition with the arguments, seed and @@ -329,6 +337,74 @@ pair, each referencing exactly the two things it pairs, or gather the pairs upstream so each is a single result. A step that seems to need a "zip" is the signal to restructure the workflow, not to add another reference. +### One step per entry: `for_each` + +A step that carries `for_each` runs once per entry of a list — a shot per +entry of `shots` — and the list is a variable the caller supplies, so a +six-shot episode is an argument rather than a different file. + +```json +{ + "name": "shot", + "for_each": "variable:shots", + "pipeline": { + "arguments": { + "prompt": "item:prompt", + "references": "item:references" + } + } +} +``` + +with + +```json +"shots": [ + { "name": "wide_open", "prompt": "the band walks on, wide", + "references": [{ "reference_type": "…", "from_previous_result": "draw_singer" }] }, + { "name": "closeup", "prompt": "closeup on the singer", + "references": [{ "reference_type": "…", "from_previous_result": "draw_singer" }] } +] +``` + +and downstream + +```json +{ "name": "edit", + "task": { "command": "concat_videos", "arguments": { "videos": "gather:shot" } } } +``` + +Before the run starts, the engine replaces the `for_each` step with one +ordinary step per entry, named `shot@wide_open`, `shot@closeup` — the +entry's `name`, or its index for an entry without one. Those are the names +the manifest, the job's events and the gallery show, and `@` is reserved +for them: a hand-written step name may not contain it. An entry's `name` +must be unique in its list and match `^[a-zA-Z_][a-zA-Z0-9_-]*$`. Give +entries names: the step cache keys on the member name, so a shot inserted +in the middle of a named list leaves every other shot cached, while an +indexed list shifts every later shot onto a different entry and regenerates +it. + +`item:field` is the whole value of that field, so an entry can carry +anything a step argument can — including a `references` list whose length +differs by shot, with `from_previous_result` and `asset:` strings inside +it. Nothing is interpolated: `"item:prompt"` is the field, `"shot: item:prompt"` +is a literal string. + +Two `for_each` steps over the *same* list are paired by name: inside +`shot@closeup`, a reference to another `for_each` step `slice` over the same +`shots` list resolves to `slice@closeup`. That is how a shot reads the audio +slice cut for it when slicing and generating are two steps. It is the one +pairing the engine has; `for_each` runs over exactly one list, and there is +no zip and no loop index. + +Limits: a list has at most 32 entries. `release_pipeline` on a `for_each` +step releases after the *last* member. Each entry is a full generation, so +quote `cost × len(list)` before running a list-driven workflow, and +`validate_workflow` with the `arguments` you will run with: it expands your +list, not the template's default, and reports a duplicate name or a missing +field at the entry's path. + ### The loop 1. `validate_workflow` — free and instant. It reports every schema error at From 0c4ebb34cbfe01035db2050982858520c76b8a74 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 18:00:21 -0500 Subject: [PATCH 09/38] test(for_each): a list-driven job end to end; proposal marks stage 1 done Co-Authored-By: Claude Fable 5.1 --- docs/proposals/list-driven-steps.md | 8 +-- tests/test_server_jobs.py | 88 +++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/docs/proposals/list-driven-steps.md b/docs/proposals/list-driven-steps.md index bf8bf8eb..a257bca8 100644 --- a/docs/proposals/list-driven-steps.md +++ b/docs/proposals/list-driven-steps.md @@ -1,10 +1,8 @@ # Proposal: list-driven steps (`for_each`) -Status: **needs approval**, revised 2026-09-11 after review. Written for MCP -feedback ticket T003; nothing here is implemented. The review found that the -first draft could not express either target template - `music-video` pairs a -slice step with a shot step per entry, and `dialogue-short`'s per-shot -reference lists vary in length - and the shape below is amended for both. +Status: **stage 1 implemented** (expansion pass, validation, schema, docs - +`dw/for_each.py`); stages 2 and 3 (templates, catalog cost and entry shape) +not started. Written for MCP feedback ticket T003. ## The ask, as filed diff --git a/tests/test_server_jobs.py b/tests/test_server_jobs.py index e24a265c..eb114c4e 100644 --- a/tests/test_server_jobs.py +++ b/tests/test_server_jobs.py @@ -121,6 +121,94 @@ def test_a_database_without_the_columns_is_migrated(tmp_path): assert row["run_id"] is None and row["run_dir"] is None +def _for_each_workflow(**overrides): + """Copied from tests/test_workflow.py's helper of the same name (Task 5) - + not imported across test modules, per that task's convention.""" + definition = { + "id": "fe", + "variables": {"shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}]}, + "steps": [ + { + "name": "shot", + "for_each": "variable:shots", + "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "result": {"content_type": "text/plain"}, + }, + { + "name": "edit", + "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "result": {"content_type": "text/plain"}, + }, + ], + } + definition.update(overrides) + return definition + + +def for_each_script(command): + """Runs the real Workflow.run() in-process - the actual for_each + expansion and compose_text execution, not a canned response - standing + in for the spawned worker process the way this file's other scripts do + (ScriptedWorkerManager replaces the process, not the workflow code).""" + from dw.workflow import workflow_from_definition + + workflow = workflow_from_definition( + command["workflow"], + command["output_dir"], + command["base_dir"], + command.get("workflow_dir"), + ) + # Mirrors dw/worker.py's _handle_execute: validated against the + # defaults first, then run() substitutes and expands the real arguments. + workflow.validate() + workflow.run(command["arguments"], {}) + yield { + "type": "success", + "message": "ok", + "run_count": 1, + "manifest": workflow.manifest, + } + + +def test_a_for_each_job_expands_and_runs_through_the_server_job_path(tmp_path): + manager = JobManager( + str(tmp_path / "outputs"), + worker_manager=ScriptedWorkerManager(for_each_script), + history_path=str(tmp_path / "jobs.sqlite"), + workflow_dir=str(tmp_path), + ) + try: + job = manager.submit( + workflow=_for_each_workflow(), + base_dir=None, + arguments={ + "shots": [ + {"name": "one", "text": "1"}, + {"name": "two", "text": "2"}, + {"name": "three", "text": "3"}, + ] + }, + ) + deadline = time.time() + 5 + while job.status not in TERMINAL_STATES and time.time() < deadline: + time.sleep(0.01) + assert job.status == "succeeded", job.error + assert [entry["step"] for entry in job.manifest] == [ + "shot@one", + "shot@two", + "shot@three", + "edit", + ] + + edit_files = job.manifest[-1]["files"] + assert len(edit_files) == 1 + edit_path = tmp_path / "outputs" / edit_files[0] + # compose_text's default separator is a blank line + assert edit_path.read_text() == "1\n\n2\n\n3" + finally: + manager.shutdown() + + def failing_script(command): """A run that writes a file, then dies on the next step - the shape of a dialogue-short whose last step names a renamed one (T015).""" From 5b30f92bc15699b1df901fda235d97eff1dce772 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 18:54:42 -0500 Subject: [PATCH 10/38] fix(for_each): copy leaves only inside a member, blame the right variable, keep source paths Three findings from the whole-branch review. expand_for_each deep-copied every leaf of every step. It runs on every run of every workflow, after realize_args has turned 'asset:' and '*_image' arguments into loaded PIL images and decoded frame lists, so that multiplied the media a run holds - and a leaf that cannot be copied (an open handle, a live model object) failed a run that had always worked. A leaf is now copied only where the copy is needed: inside a member, where one template value is about to appear in every one of them, and there the copy falls back to the object itself when deepcopy raises, the choice the step cache already makes. The 'input untouched' contract holds because the pass never mutates a leaf. An unrelated undeclared 'variable:' made the pre-flight blame for_each: replace_variables raises on the first one, and the whole definition was then left unsubstituted, so a perfectly good 'shots' list reached the expansion as the literal string 'variable:shots' and the error said the list was never substituted - false, and it hid the actual typo. validation_errors now reports every undeclared reference at the path it sits at. That is an error rather than a warning because it is always fatal: once a workflow declares variables, replace_variables refuses an undeclared reference, so it is a run that cannot start. The check found one in workflows/templates/inpaint.json, which declared 'mage'. Reference errors on the expanded definition rendered expanded step indices - a path that exists in no file the author wrote. expand_for_each now records each expanded step's source index in a caller-supplied list (not a key on the step: step_data is what the step cache keys on and what the schema validates), and previous_result_reference_errors renders that index, naming the member in the message so the reader knows which expansion failed. Co-Authored-By: Claude Fable 5.1 --- dw/for_each.py | 46 +++++++++-- dw/previous_results.py | 34 ++++++-- dw/workflow.py | 52 ++++++++++--- tests/test_for_each.py | 128 ++++++++++++++++++++++++++++--- tests/test_integration.py | 6 +- tests/test_library_sources.py | 2 +- tests/test_server_jobs.py | 14 +++- tests/test_validate_arguments.py | 10 ++- tests/test_workflow.py | 70 ++++++++++++++++- workflows/templates/inpaint.json | 2 +- 10 files changed, 317 insertions(+), 47 deletions(-) diff --git a/dw/for_each.py b/dw/for_each.py index 5c554e36..5a1191f3 100644 --- a/dw/for_each.py +++ b/dw/for_each.py @@ -57,11 +57,18 @@ def member_name(group, key): return f"{group}{MEMBER_SEPARATOR}{key}" -def expand_for_each(definition): +def expand_for_each(definition, source_indices=None): """The definition with every 'for_each' step replaced by its members. Returns a new structure; `definition` is left as it was passed in. Raises ForEachError for anything that cannot be expanded. + + `source_indices`, when a list is passed, has the index of the step each + expanded step was written as appended to it, so a later check can report + an error at a path in the file the author wrote rather than at an + expanded index that exists nowhere. A parallel list rather than a key on + the step: step_data is what the step cache keys on and what the schema + validates, and neither may learn a new field. """ steps = definition.get("steps") if isinstance(definition, dict) else None if not isinstance(steps, list): @@ -75,6 +82,7 @@ def expand_for_each(definition): for index, step in enumerate(steps): if not isinstance(step, dict): expanded.append(copy.deepcopy(step)) + _record(source_indices, index) continue path = ("steps", index) name = step.get("name") @@ -86,6 +94,7 @@ def expand_for_each(definition): ) if FOR_EACH_KEY not in step: expanded.append(_rewrite(step, path, groups, member=None)) + _record(source_indices, index) continue entries = step[FOR_EACH_KEY] @@ -106,6 +115,7 @@ def expand_for_each(definition): for flag in _LAST_MEMBER_ONLY: expanded_step.pop(flag, None) expanded.append(expanded_step) + _record(source_indices, index) groups[name] = {"keys": keys, "entries": entries} result = {k: v for k, v in definition.items() if k != "steps"} @@ -113,6 +123,11 @@ def expand_for_each(definition): return result +def _record(source_indices, index): + if source_indices is not None: + source_indices.append(index) + + def _entry_keys(entries, path): """The key of every entry - its 'name' when it is an object carrying one, else its index - validated and unique.""" @@ -122,7 +137,8 @@ def _entry_keys(entries, path): else: hint = "" raise ForEachError( - render_path(path), f"for_each must be a list, got {type(entries).__name__}{hint}" + render_path(path), + f"for_each must be a list, got {type(entries).__name__}{hint}", ) if len(entries) > MAX_FOR_EACH_ENTRIES: raise ForEachError( @@ -181,7 +197,27 @@ def _rewrite(value, path, groups, member): reference, path, groups, member ) return value - return copy.deepcopy(value) + # A leaf is only copied where the copy is needed: inside a member, where + # the same template value is about to appear in every one of them. + # Outside, the leaf is handed back as it is - this pass runs on every run + # of every workflow, after realize_args has turned 'asset:' arguments + # into loaded images and decoded frame lists, and copying all of that + # would multiply the media a run holds. The 'input untouched' contract + # still holds because nothing here ever mutates a leaf + return _copy_leaf(value) if member is not None else value + + +def _copy_leaf(value): + """A copy of a leaf, or the leaf itself when it cannot be copied. + + An open handle or a live model object reaching a member is not a reason + to fail a run - the step cache makes the same choice for a realized + argument it cannot deep-copy (dw/workflow.py). + """ + try: + return copy.deepcopy(value) + except Exception: + return value def _item(value, path, member): @@ -192,7 +228,7 @@ def _item(value, path, member): field = value[len(ITEM_PREFIX) :] entry = member["entry"] if field == "": - return copy.deepcopy(entry) + return _copy_leaf(entry) if not isinstance(entry, dict): raise ForEachError( render_path(path), @@ -205,7 +241,7 @@ def _item(value, path, member): f"'{value}' names no field of entry '{member['key']}' of for_each " f"step '{member['group']}'; it has: {sorted(entry)}", ) - return copy.deepcopy(entry[field]) + return _copy_leaf(entry[field]) def _gather(value, path, groups): diff --git a/dw/previous_results.py b/dw/previous_results.py index 3fa0a2e1..3a093f37 100644 --- a/dw/previous_results.py +++ b/dw/previous_results.py @@ -6,7 +6,7 @@ PREVIOUS_RESULT_PREFIX, build_objects, ) -from .for_each import render_path +from .for_each import MEMBER_SEPARATOR, render_path from .step_cache import reference_resolves_to logger = logging.getLogger("dw") @@ -309,7 +309,7 @@ def _not_found(previous_results, previous_result_name): return KeyError(message) -def previous_result_reference_errors(workflow_definition): +def previous_result_reference_errors(workflow_definition, source_indices=None): """Every 'previous_result:' reference that names no earlier step. References resolve lazily, one step at a time, so a reference naming a @@ -319,8 +319,14 @@ def previous_result_reference_errors(workflow_definition): a read of the file would have caught (T005). The names are all in the definition, so this is answerable before anything runs. - Only literal references are checked: one spelled by a 'variable:' the - caller supplies is not knowable here and is left alone. + The definition handed here has already been substituted and expanded, + so every reference in it is literal; a 'variable:' still spelled out is + one nothing resolved and is left alone. + + `source_indices`, when given, is the source step index of each step - + expansion of a 'for_each' group turns one written step into several, and + the path an error carries has to be one the author can find in the file + they wrote. """ steps = workflow_definition.get("steps") if not isinstance(steps, list): @@ -331,22 +337,34 @@ def previous_result_reference_errors(workflow_definition): for index, step in enumerate(steps): if not isinstance(step, dict): continue + name = step.get("name") found = {} _collect_reference_paths(step, (), found) for path, reference in sorted(found.items(), key=lambda item: str(item[0])): if any(reference_resolves_to(reference, name) for name in seen): continue - location = render_path(("steps", index) + path) + source = ( + source_indices[index] + if source_indices is not None and index < len(source_indices) + else index + ) + location = render_path(("steps", source) + path) + # Which expansion it was: the source path alone points at the one + # step the author wrote, and every member reports the same path + where = ( + f" in member '{name}'" + if isinstance(name, str) and MEMBER_SEPARATOR in name + else "" + ) errors.append( { "path": location, "message": ( - f"previous_result '{reference}' names no earlier step. " - f"Steps available here: {seen}" + f"previous_result '{reference}'{where} names no earlier " + f"step. Steps available here: {seen}" ), } ) - name = step.get("name") if isinstance(name, str): seen.append(name) return errors diff --git a/dw/workflow.py b/dw/workflow.py index d0d3ce63..0ce358c4 100644 --- a/dw/workflow.py +++ b/dw/workflow.py @@ -47,6 +47,7 @@ argument_errors, replace_variables, set_variables, + undeclared_variable_references, VariableNotFoundError, ) from .pipeline_processors.pipeline import Pipeline @@ -288,26 +289,27 @@ def effective_output_dir(self): os.path.join(self.output_dir, subfolder) if subfolder else self.output_dir ) - def expanded_definition(self, arguments=None): + def expanded_definition(self, arguments=None, source_indices=None): """The definition as the run will see it: variables substituted - the caller's `arguments` folded in when they are all good, else the declared defaults - and every for_each step expanded. - Raises ForEachError for a for_each that cannot be expanded. A - 'variable:' that names nothing is left in place rather than raised: - validate_workflow already reports that as a warning, and the - reference check is happy to skip a reference it cannot read. + Raises ForEachError for a for_each that cannot be expanded, and + VariableNotFoundError for a 'variable:' that names nothing - which + is exactly what the run itself would raise, since a definition that + declares variables is always substituted before it runs. + + `source_indices`, when a list is passed, comes back holding the + index in *this* definition's steps of every expanded step, so an + error can be reported at a path in the file the author wrote. """ definition = copy.deepcopy(self.workflow_definition) variables = definition.get("variables") if isinstance(variables, dict): if arguments and not argument_errors(definition, arguments): set_variables(arguments, variables) - try: - definition = replace_variables(definition, variables) - except VariableNotFoundError: - pass - return expand_for_each(definition) + definition = replace_variables(definition, variables) + return expand_for_each(definition, source_indices) def validation_errors(self, arguments=None): """Every schema violation in the definition, as [{path, message}]; @@ -319,11 +321,37 @@ def validation_errors(self, arguments=None): # such array to walk if errors: return errors + source_indices = [] try: - expanded = self.expanded_definition(arguments) + expanded = self.expanded_definition(arguments, source_indices) except ForEachError as e: return [{"path": e.path, "message": str(e)}] - return previous_result_reference_errors(expanded) + except VariableNotFoundError: + # Every undeclared reference, not just the first one substitution + # tripped over - and reported where each sits rather than as a + # for_each whose list arrived unsubstituted, which is what a + # half-substituted definition used to look like from here + return self._undeclared_variable_errors() + return previous_result_reference_errors(expanded, source_indices) + + def _undeclared_variable_errors(self): + """Every 'variable:' reference naming nothing the workflow declares. + + Fatal rather than a warning: once a workflow has a 'variables' + block, replace_variables refuses an undeclared reference, so this is + a run that cannot start. + """ + declared = sorted(self.workflow_definition.get("variables") or {}) + return [ + { + "path": path, + "message": ( + f"'variable:{name}' names no declared variable; " + f"declared: {', '.join(declared) or ''}" + ), + } + for path, name in undeclared_variable_references(self.workflow_definition) + ] def validate(self): """Validates workflow definition against JSON schema. diff --git a/tests/test_for_each.py b/tests/test_for_each.py index 813782dd..b26d1d96 100644 --- a/tests/test_for_each.py +++ b/tests/test_for_each.py @@ -273,6 +273,80 @@ def test_members_do_not_share_mutable_values(self): assert second["pipeline"]["arguments"]["references"][0]["k"] == 1 +class TestSourceIndices: + """The expansion records where each step came from in the source file. + + A parallel list rather than a key on the step: step_data is what the + step cache keys on and what the schema validates, so nothing new may + appear in it. + """ + + def test_every_expanded_step_records_its_source_index(self): + source_indices = [] + expanded = expand_for_each( + definition( + {"name": "draw", "task": {}}, + {"name": "shot", "for_each": ["a", "b", "c"], "task": {}}, + {"name": "edit", "task": {}}, + ), + source_indices, + ) + assert [s["name"] for s in expanded["steps"]] == [ + "draw", + "shot@0", + "shot@1", + "shot@2", + "edit", + ] + assert source_indices == [0, 1, 1, 1, 2] + + def test_the_list_is_optional(self): + expanded = expand_for_each( + definition({"name": "shot", "for_each": ["a"], "task": {}}) + ) + assert [s["name"] for s in expanded["steps"]] == ["shot@0"] + + +class TestLeafCopying: + """A leaf is copied only where the copy is needed - inside a member. + + expand_for_each runs on every run of every workflow, after realize_args + has turned 'asset:' and '*_image' arguments into loaded PIL images and + decoded frame lists. Copying every leaf of every step would multiply + that media, and a leaf that cannot be copied at all would fail a run + that has always worked. + """ + + class Uncopyable: + def __deepcopy__(self, memo): + raise TypeError("cannot copy this") + + def test_a_leaf_outside_a_member_is_passed_through_by_identity(self): + leaf = self.Uncopyable() + expanded = expand_for_each( + definition({"name": "plain", "task": {"arguments": {"model": leaf}}}) + ) + assert expanded["steps"][0]["task"]["arguments"]["model"] is leaf + + def test_a_leaf_inside_a_member_that_cannot_be_copied_is_the_object_itself(self): + # The step cache makes the same choice for a realized argument it + # cannot deep-copy: the run goes on, with the object as it is + leaf = self.Uncopyable() + expanded = expand_for_each( + definition( + { + "name": "shot", + "for_each": ["a", "b"], + "task": {"arguments": {"model": leaf}}, + } + ) + ) + assert [s["task"]["arguments"]["model"] for s in expanded["steps"]] == [ + leaf, + leaf, + ] + + class TestRelease: def test_release_flags_survive_on_the_last_member_only(self): expanded = expand_for_each( @@ -369,7 +443,10 @@ def test_gather_in_a_sub_workflow_argument_map(self): self.group( { "name": "score", - "workflow": {"path": "builtin:x.json", "arguments": {"clips": "gather:shot"}}, + "workflow": { + "path": "builtin:x.json", + "arguments": {"clips": "gather:shot"}, + }, } ) ) @@ -446,7 +523,9 @@ def test_a_variable_spelled_from_previous_result_is_untouched(self): definition( { "name": "edit", - "task": {"arguments": {"r": [{"from_previous_result": "variable:x"}]}}, + "task": { + "arguments": {"r": [{"from_previous_result": "variable:x"}]} + }, } ) ) @@ -557,8 +636,16 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): template = load_template("music-video.json") today = steps_by_name(template) shots = [ - {"name": "wide_open", "prompt": "variable:shot_1_wide_open", "start_frame": 0}, - {"name": "closeup", "prompt": "variable:shot_2_closeup", "start_frame": 124}, + { + "name": "wide_open", + "prompt": "variable:shot_1_wide_open", + "start_frame": 0, + }, + { + "name": "closeup", + "prompt": "variable:shot_2_closeup", + "start_frame": 124, + }, {"name": "room", "prompt": "variable:shot_3_room", "start_frame": 248}, {"name": "finale", "prompt": "variable:shot_4_finale", "start_frame": 372}, ] @@ -580,8 +667,13 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): expanded = expand_for_each( definition( - today["draw_singer"], today["write_song"], slice_template, - today["soundtrack"], shot_template, edit, today["music_video"], + today["draw_singer"], + today["write_song"], + slice_template, + today["soundtrack"], + shot_template, + edit, + today["music_video"], ) ) got = steps_by_name(expanded) @@ -594,8 +686,15 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): # Each expanded shot is today's shot (as a full pipeline block) with # the new name and its slice renamed - hand_written = ["shot_1_wide_open", "shot_2_closeup", "shot_3_room", "shot_4_finale"] - for key, old, index in zip(["wide_open", "closeup", "room", "finale"], hand_written, range(1, 5)): + hand_written = [ + "shot_1_wide_open", + "shot_2_closeup", + "shot_3_room", + "shot_4_finale", + ] + for key, old, index in zip( + ["wide_open", "closeup", "room", "finale"], hand_written, range(1, 5) + ): step = today[old] if "pipeline_reference" in step: step = without_pipeline_reference(step, today["shot_1_wide_open"]) @@ -607,7 +706,8 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): assert got[f"shot@{key}"] == expected assert got["edit"]["task"]["arguments"]["videos"] == [ - f"previous_result:shot@{k}" for k in ["wide_open", "closeup", "room", "finale"] + f"previous_result:shot@{k}" + for k in ["wide_open", "closeup", "room", "finale"] ] @@ -628,7 +728,11 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): ] first = today["shot_1_cold_open"] full = { - old: (step if "pipeline_reference" not in step else without_pipeline_reference(step, first)) + old: ( + step + if "pipeline_reference" not in step + else without_pipeline_reference(step, first) + ) for old, step in today.items() if old.startswith("shot_") } @@ -653,7 +757,9 @@ def test_the_hand_written_shots_are_what_the_list_expands_to(self): shot_template["pipeline"]["arguments"]["num_frames"] = "item:num_frames" expanded = expand_for_each( - definition(today["draw_character_a"], today["draw_character_b"], shot_template) + definition( + today["draw_character_a"], today["draw_character_b"], shot_template + ) ) got = steps_by_name(expanded) for key, old in hand_written: diff --git a/tests/test_integration.py b/tests/test_integration.py index 04e3d8b4..0d22f501 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -165,7 +165,11 @@ def test_missing_variable_reference(self, temp_workflow_dir): } workflow = Workflow(workflow_data, temp_workflow_dir, "") - workflow.validate() + # Fatal at run time, so validation reports it rather than letting the + # run reach the step that spells it + with pytest.raises(Exception) as validation_error: + workflow.validate() + assert "undefined_var" in str(validation_error.value) with pytest.raises(Exception) as exc_info: workflow.run({}) diff --git a/tests/test_library_sources.py b/tests/test_library_sources.py index 01f14b5c..b73f969b 100644 --- a/tests/test_library_sources.py +++ b/tests/test_library_sources.py @@ -230,7 +230,7 @@ def test_validated_arguments_reach_the_whole_asset_path(self, client, tmp_path): the caller works in reads as though it was never looked in.""" api, workspace, checkout = client definition = valid_workflow("refs") - definition["variables"] = {"image": "asset:mine.png"} + definition["variables"]["image"] = "asset:mine.png" for name in ("asset:mine.png", "asset:iris.png"): answer = api.post( diff --git a/tests/test_server_jobs.py b/tests/test_server_jobs.py index eb114c4e..ed8504bd 100644 --- a/tests/test_server_jobs.py +++ b/tests/test_server_jobs.py @@ -126,17 +126,25 @@ def _for_each_workflow(**overrides): not imported across test modules, per that task's convention.""" definition = { "id": "fe", - "variables": {"shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}]}, + "variables": { + "shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}] + }, "steps": [ { "name": "shot", "for_each": "variable:shots", - "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "task": { + "command": "compose_text", + "arguments": {"parts": ["item:text"]}, + }, "result": {"content_type": "text/plain"}, }, { "name": "edit", - "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "task": { + "command": "compose_text", + "arguments": {"parts": "gather:shot"}, + }, "result": {"content_type": "text/plain"}, }, ], diff --git a/tests/test_validate_arguments.py b/tests/test_validate_arguments.py index 3eb99f28..2dbfe0a6 100644 --- a/tests/test_validate_arguments.py +++ b/tests/test_validate_arguments.py @@ -170,12 +170,18 @@ def test_validate_expands_for_each_with_the_callers_list(self, server): { "name": "shot", "for_each": "variable:shots", - "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "task": { + "command": "compose_text", + "arguments": {"parts": ["item:text"]}, + }, "result": {"content_type": "text/plain"}, }, { "name": "edit", - "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "task": { + "command": "compose_text", + "arguments": {"parts": "gather:shot"}, + }, "result": {"content_type": "text/plain"}, }, ], diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 2241bb02..cf2476fb 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -622,17 +622,25 @@ def _workflow_from(definition, tmp_path): def _for_each_workflow(**overrides): definition = { "id": "fe", - "variables": {"shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}]}, + "variables": { + "shots": [{"name": "a", "text": "A"}, {"name": "b", "text": "B"}] + }, "steps": [ { "name": "shot", "for_each": "variable:shots", - "task": {"command": "compose_text", "arguments": {"parts": ["item:text"]}}, + "task": { + "command": "compose_text", + "arguments": {"parts": ["item:text"]}, + }, "result": {"content_type": "text/plain"}, }, { "name": "edit", - "task": {"command": "compose_text", "arguments": {"parts": "gather:shot"}}, + "task": { + "command": "compose_text", + "arguments": {"parts": "gather:shot"}, + }, "result": {"content_type": "text/plain"}, }, ], @@ -677,6 +685,62 @@ def test_validation_of_an_undeclared_variable_still_does_not_raise(tmp_path): workflow = _workflow_from(definition, tmp_path) errors = workflow.validation_errors() assert errors and "variable:missing" in errors[0]["message"] + assert errors[0]["path"] == "steps[0].for_each" + + +def test_an_unrelated_undeclared_variable_is_not_blamed_on_for_each(tmp_path): + """A typo in one step used to leave the whole definition unsubstituted, + so a perfectly good for_each list arrived as the literal + 'variable:shots' and the error blamed the list the author got right.""" + definition = _for_each_workflow() + definition["steps"][0]["task"]["arguments"]["parts"] = [ + "item:text", + "variable:promt", + ] + errors = _workflow_from(definition, tmp_path).validation_errors() + assert len(errors) == 1 + assert errors[0]["path"] == "steps[0].task.arguments.parts[1]" + assert "promt" in errors[0]["message"] and "shots" in errors[0]["message"] + assert "for_each" not in errors[0]["message"] + + +def test_a_reference_error_after_a_group_names_the_step_in_the_file(tmp_path): + """The expansion moves the later steps along; the path the author reads + has to be the one in the file they wrote, not the expanded index.""" + definition = _for_each_workflow() + definition["variables"]["shots"] = [ + {"name": "a", "text": "A"}, + {"name": "b", "text": "B"}, + {"name": "c", "text": "C"}, + ] + definition["steps"].insert( + 0, + { + "name": "draw", + "task": {"command": "compose_text", "arguments": {"parts": ["x"]}}, + "result": {"content_type": "text/plain"}, + }, + ) + definition["steps"][2]["task"]["arguments"]["parts"] = ["previous_result:nope"] + errors = _workflow_from(definition, tmp_path).validation_errors() + assert len(errors) == 1 + assert errors[0]["path"] == "steps[2].task.arguments.parts[0]" + + +def test_a_reference_error_inside_a_member_names_the_member(tmp_path): + definition = _for_each_workflow() + definition["steps"][0]["task"]["arguments"]["parts"] = [ + "item:text", + {"from_previous_result": "nope"}, + ] + errors = _workflow_from(definition, tmp_path).validation_errors() + # One per member, each at the source step's path + assert [e["path"] for e in errors] == [ + "steps[0].task.arguments.parts[1].from_previous_result", + "steps[0].task.arguments.parts[1].from_previous_result", + ] + assert "in member 'shot@a'" in errors[0]["message"] + assert "in member 'shot@b'" in errors[1]["message"] def test_run_expands_for_each_and_names_the_members(tmp_path): diff --git a/workflows/templates/inpaint.json b/workflows/templates/inpaint.json index 7d0d18eb..abc62680 100644 --- a/workflows/templates/inpaint.json +++ b/workflows/templates/inpaint.json @@ -1,6 +1,6 @@ { "variables": { - "mage": "https://huggingface.co/datasets/diffusers/diffusers-images-docs/resolve/main/cup.png", + "image": "https://huggingface.co/datasets/diffusers/diffusers-images-docs/resolve/main/cup.png", "mask_image": "https://huggingface.co/datasets/diffusers/diffusers-images-docs/resolve/main/cup_mask.png" }, "id": "FluxFill", From 69ce9836c1fcaffa2b130d1ad2eb6e23343ba5da Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 18:54:42 -0500 Subject: [PATCH 11/38] docs(for_each): the new validation contract, paired by key, stage-2 notes CLAUDE.md's older previous_result bullet still said a reference spelled by a 'variable:' is left alone, which the substitute-then-check order has made false in both directions: a declared variable's reference is checked by its value, and an undeclared one is itself an error now. The guide said two for_each steps over the same list are paired by name; the mechanism is by key - the entry's name, else its index - and it now states where an error's path points, since expansion no longer leaks expanded indices. The proposal records four observations for the stage-2 template rewrite: pipeline_reference naming a group gets no directed error, an entry field named image/*_image/*_video is realized before expansion, an empty list expands to no steps at all, and expanded_definition skips realize_constants. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 15 ++++++++++----- docs/WORKFLOW_GUIDE.md | 11 ++++++++--- docs/proposals/list-driven-steps.md | 27 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c0b7ebff..bbbca073 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -230,14 +230,19 @@ same reason - default setup cannot load a pack. literal `previous_result:` or `from_previous_result` naming no *earlier* step, with the JSON path it sits at. References otherwise resolve lazily per step, so a step renamed in one place and not another failed only when the run reached it, after - every step before it had generated. A reference spelled by a `variable:` is left - alone - what it names is not knowable before substitution + every step before it had generated. The definition is substituted before the check, + so a reference spelled by a *declared* variable is checked by its value; one spelled + by an undeclared variable is itself a validation error (below) - **`for_each` expands before the reference check** — `validation_errors` substitutes (the caller's `arguments` when they are all good, else the defaults) and expands first, so `gather:` and `item:` errors carry the path of the template step - (`steps[0].for_each[1].name`) while a bad reference inside a member carries the - member's. Since substitution now precedes the check, a `from_previous_result` - spelled by a *declared* variable is checked by its value + (`steps[0].for_each[1].name`). Expansion records each expanded step's *source* index + (`expand_for_each(definition, source_indices)`), so a reference error always carries a + path in the file the author wrote, and one inside a member names the member in its + message. An undeclared `variable:` is a validation error at the path it sits at, not a + warning and not a complaint about the `for_each` list that did substitute: once a + `variables` block exists, `replace_variables` refuses an undeclared reference, so it is + a run that cannot start - **Cartesian product explosion** — multiple `previous_result` references multiply: 4 images × 3 masks = 12 iterations - **Component sharing requires exact key matching** between `shared_components` and `reused_components` - **Built-in workflows** need explicit argument mapping: `"prompt": "variable:prompt"` diff --git a/docs/WORKFLOW_GUIDE.md b/docs/WORKFLOW_GUIDE.md index de294df5..e073a17b 100644 --- a/docs/WORKFLOW_GUIDE.md +++ b/docs/WORKFLOW_GUIDE.md @@ -391,9 +391,10 @@ differs by shot, with `from_previous_result` and `asset:` strings inside it. Nothing is interpolated: `"item:prompt"` is the field, `"shot: item:prompt"` is a literal string. -Two `for_each` steps over the *same* list are paired by name: inside -`shot@closeup`, a reference to another `for_each` step `slice` over the same -`shots` list resolves to `slice@closeup`. That is how a shot reads the audio +Two `for_each` steps over the *same* list are paired by key — the entry's +`name`, or its index for an entry without one: inside `shot@closeup`, a +reference to another `for_each` step `slice` over the same `shots` list +resolves to `slice@closeup`. That is how a shot reads the audio slice cut for it when slicing and generating are two steps. It is the one pairing the engine has; `for_each` runs over exactly one list, and there is no zip and no loop index. @@ -405,6 +406,10 @@ quote `cost × len(list)` before running a list-driven workflow, and list, not the template's default, and reports a duplicate name or a missing field at the entry's path. +Every error carries a path in the file you wrote, not in the expanded step +list: a bad reference inside a member is reported at the `for_each` step's +own path, with the member it failed in named in the message. + ### The loop 1. `validate_workflow` — free and instant. It reports every schema error at diff --git a/docs/proposals/list-driven-steps.md b/docs/proposals/list-driven-steps.md index a257bca8..cf50bde9 100644 --- a/docs/proposals/list-driven-steps.md +++ b/docs/proposals/list-driven-steps.md @@ -290,3 +290,30 @@ mechanical once it exists. flag was the first idea and cannot produce a references list whose length varies by shot. The template question stage 2 has to get right is how much of that list the entry writes and how much the template fixes. + +## Notes for stage 2 + +Observations from the stage-1 review, recorded so the template rewrite does +not have to rediscover them: + +- **`pipeline_reference.reference_name` naming a `for_each` group gets no + directed error.** Both templates use `pipeline_reference` on their shot + steps, and a reference to a group's name resolves to nothing helpful - the + expansion only rewrites `previous_result:` and `from_previous_result`. + Rewriting a shot step onto `for_each` means either pointing every member's + `pipeline_reference` at a step outside the group, or giving that key the + same treatment. +- **An entry field named `image`/`*_image`/`*_video` is realized before + expansion.** `realize_args` runs over the variables, so such a field is a + loaded PIL image or a decoded frame list by the time `item:image` splices + it into a member. It works, it is untested, and it is why the expansion + copies a leaf only inside a member and falls back to the object itself + when it cannot be copied. +- **A `for_each` over an empty list expands to zero steps.** A workflow whose + only step is that one returns before `workflow_start`, so the run produces + no events and no manifest entries. Whether that is an error or an empty + success is a stage-2 decision. +- **`expanded_definition` skips `realize_constants`.** A list defaulted to a + `constant:` name fails validation (the list arrives as the `constant:` + string) and then runs fine, since `Workflow.run` does realize constants. A + template that wants a constant default needs that pass in the validator. From 2ca4eb8e6912949e109b975a15e6e011f5912b9b Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:21:33 -0500 Subject: [PATCH 12/38] plan: for_each stage 2 - the cut templates on a shots list Co-Authored-By: Claude Fable 5.1 --- .../plans/2026-09-11-list-driven-templates.md | 1094 +++++++++++++++++ 1 file changed, 1094 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-11-list-driven-templates.md diff --git a/docs/superpowers/plans/2026-09-11-list-driven-templates.md b/docs/superpowers/plans/2026-09-11-list-driven-templates.md new file mode 100644 index 00000000..942c21d2 --- /dev/null +++ b/docs/superpowers/plans/2026-09-11-list-driven-templates.md @@ -0,0 +1,1094 @@ +# List-driven templates (for_each stage 2) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Rewrite `workflows/templates/minimax/music-video.json` and `workflows/templates/minimax/dialogue-short.json` onto one `shots` list each, so a six-shot episode is an argument rather than a different file, and move the plugin skill, docs and tests with them. + +**Architecture:** Stage 1 shipped `expand_for_each` (`dw/for_each.py`): a step carrying `for_each` becomes one ordinary step per entry, `item:field` splices an entry's field, `gather:` reads the group. This stage (1) lets a list-valued variable's entries reference *other* variables (`"from_file": "variable:character_a_voice"`), resolved once before `realize_args`, because that is how dialogue-short's optional voices survive the move into entries; (2) teaches the catalog's shape derivation that `"videos": "gather:shot"` is a cut over a list; (3) rewrites the two templates, whose shot steps become full `pipeline` blocks (the identity-keyed pipeline cache means every member after the first reuses the loaded H3 exactly as `pipeline_reference` did); (4) rewrites the tests that hand-expanded the *old* templates, the voices test, the plugin skill and the docs. + +**Tech Stack:** Python 3.10, pytest, JSON workflow templates, markdown skill/docs. + +**Spec:** `docs/proposals/list-driven-steps.md` — "Phasing" item 2, "Decided at review", and "Notes for stage 2". + +## Global Constraints + +- Every test in the suite passes: `pytest -q -x tests/` (the baseline on `develop` at faa27e1 is 3492 passed, 5 skipped). Run the full suite before every commit that touches `dw/`. +- No engine behaviour changes beyond the two named here: nested `variable:` resolution inside list/dict variable values, and `gather:` recognised by `_cuts_together`. Nothing else in `dw/for_each.py`, `dw/variables.py` or `dw/workflow.py` changes semantics. +- The rewritten templates must expand (`Workflow.expanded_definition()`) to the same *number* of shot steps with the same prompts, references (same order, same `from_previous_result` targets), `num_frames` and `start_frame` values the hand-written steps carried on `develop` at faa27e1 — `git show faa27e1:workflows/templates/minimax/.json` is the reference. +- Member names are `slice@` / `shot@` with keys `wide_open`, `closeup`, `room`, `finale` (music-video) and `cold_open`, `deflect`, `react`, `button`, `tag` (dialogue-short) — the entry names stage 1's tests already chose. +- The catalog pins hold unchanged: `tests/test_catalog_structure.py` `EXPECTED_SHAPES` (`sequence`, `["has-audio", "identity-referenced"]`) and `COSTED` (35 / 42 minutes) for both templates. +- `plugins/dw/skills/minimax-h3/SKILL.md` stays at or under 12288 bytes (`SKILL_SIZE_LIMIT`); it is at 11535 now, so every sentence added is paid for by one removed or tightened. +- Security rules from CLAUDE.md apply: entry names are already validated by `validate_variable_name` in the expansion; nothing new touches the filesystem. +- Commit trailer on every commit: `Co-Authored-By: Claude Fable 5.1 `. +- No version bump in this branch. The proposal calls for the two templates to land together, and they do; the engine version (`pyproject.toml`, `plugins/dw/.claude-plugin/plugin.json`, both `0.4.0-beta.3`) is bumped by `scripts/release.sh` from master, which is the user's call. The breaking change (the `shot_N_*` argument names are gone) is recorded in the proposal's status and in the CLAUDE.md gotcha so the next release note can quote it. + +--- + +### Task 1: A list-valued variable's entries may reference other variables + +**Files:** +- Modify: `dw/variables.py` (add `resolve_variable_values`; extend `undeclared_variable_references`) +- Modify: `dw/workflow.py:300-312` (`expanded_definition`), `dw/workflow.py:337-354` (`_undeclared_variable_errors`), `dw/workflow.py:430-446` (`Workflow.run` substitution block) +- Test: `tests/test_variables.py`, `tests/test_workflow.py` + +**Interfaces:** +- Consumes: `replace_variables(data, variables)`, `VariableNotFoundError`, `set_variables`, `argument_errors` (all in `dw/variables.py`). +- Produces: `resolve_variable_values(variables) -> dict` — a new dict in which every `"variable:"` string found *inside* a list- or dict-valued variable is replaced by that variable's (already resolved) value. Scalar variable values are returned as they are, even a string that begins with `variable:` (that has always been passed through verbatim and stays so). Raises `VariableNotFoundError` for an undeclared name and `ValueError("Variable 'a' references itself through: a -> b -> a")` for a cycle. `undeclared_variable_references(definition)` now also returns references found inside list/dict values of `variables`, with paths like `variables.shots[0].references[2].from_file`. `Workflow._undeclared_variable_errors(arguments=None)` folds good caller arguments in before walking, and reports a reference inside a caller-supplied value at `arguments....` rather than `variables....`. + +- [ ] **Step 1: Write the failing tests for `resolve_variable_values`** + +Append to `tests/test_variables.py`: + +```python +from dw.variables import resolve_variable_values, undeclared_variable_references + + +class TestResolveVariableValues: + """A list-valued variable's entries may name other variables - a shot + entry says "from_file": "variable:character_a_voice" and one variable + sets the voice in every shot it speaks in.""" + + def test_a_reference_inside_a_list_value_is_replaced(self): + variables = { + "voice": "cast/priya.wav", + "shots": [{"name": "a", "references": [{"from_file": "variable:voice"}]}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"][0]["from_file"] == "cast/priya.wav" + + def test_a_reference_inside_a_dict_value_is_replaced(self): + variables = {"n": 124, "shape": {"num_frames": "variable:n"}} + assert resolve_variable_values(variables)["shape"] == {"num_frames": 124} + + def test_a_null_variable_resolves_to_null(self): + variables = { + "voice": None, + "shots": [{"references": [{"from_file": "variable:voice"}]}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"][0]["from_file"] is None + + def test_a_scalar_value_that_looks_like_a_reference_is_left_alone(self): + variables = {"x": "variable:y", "y": 1} + assert resolve_variable_values(variables)["x"] == "variable:y" + + def test_a_chain_resolves_through_a_referenced_list(self): + variables = { + "voice": "a.wav", + "refs": [{"from_file": "variable:voice"}], + "shots": [{"references": "variable:refs"}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"] == [{"from_file": "a.wav"}] + + def test_the_input_is_not_mutated(self): + variables = {"voice": "a.wav", "shots": [{"from_file": "variable:voice"}]} + before = copy.deepcopy(variables) + resolve_variable_values(variables) + assert variables == before + + def test_an_undeclared_name_is_the_usual_error(self): + with pytest.raises(VariableNotFoundError, match="nope"): + resolve_variable_values({"shots": [{"x": "variable:nope"}]}) + + def test_a_cycle_is_an_error_that_names_the_loop(self): + variables = {"a": [{"x": "variable:b"}], "b": [{"y": "variable:a"}]} + with pytest.raises(ValueError, match="a -> b -> a"): + resolve_variable_values(variables) + + def test_a_self_reference_is_a_cycle(self): + with pytest.raises(ValueError, match="a -> a"): + resolve_variable_values({"a": [{"x": "variable:a"}]}) + + +class TestUndeclaredReferencesInsideVariableValues: + def test_a_reference_inside_a_list_value_is_found_with_its_path(self): + definition = { + "variables": {"shots": [{"references": [{}, {"from_file": "variable:nope"}]}]}, + "steps": [], + } + assert undeclared_variable_references(definition) == [ + ("variables.shots[0].references[1].from_file", "nope") + ] + + def test_a_declared_reference_inside_a_value_is_not_reported(self): + definition = { + "variables": {"voice": None, "shots": [{"from_file": "variable:voice"}]}, + "steps": [], + } + assert undeclared_variable_references(definition) == [] + + def test_a_scalar_value_beginning_with_the_prefix_is_not_a_reference(self): + definition = {"variables": {"x": "variable:nope"}, "steps": []} + assert undeclared_variable_references(definition) == [] +``` + +Add `import copy` and `import pytest` at the top of the file if they are not already there, and make sure `VariableNotFoundError` is imported from `dw.variables`. + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `pytest tests/test_variables.py -q` +Expected: ImportError on `resolve_variable_values`. + +- [ ] **Step 3: Implement `resolve_variable_values` and extend the undeclared walk** + +In `dw/variables.py`, after `replace_variables`: + +```python +def resolve_variable_values(variables): + """A copy of `variables` in which every "variable:name" inside a list- + or dict-valued variable is replaced by that variable's value. + + A list-driven step reads its entries from a variable, and an entry that + says "from_file": "variable:character_a_voice" is how one variable sets + a voice in every shot the character speaks in. replace_variables only + walks the definition, so those references would reach the step as the + literal strings; this resolves them once, before realize_args, so a + reference type inside an entry is a type name by the time it is loaded. + + Only list and dict values are walked. A scalar value that begins with + "variable:" is passed through as it always was. + + Raises: + VariableNotFoundError: a reference names nothing declared + ValueError: a value references itself, directly or through others + """ + resolved = {} + + def resolve(name, chain): + if name in resolved: + return resolved[name] + if name in chain: + loop = " -> ".join(chain[chain.index(name) :] + [name]) + raise ValueError(f"Variable '{name}' references itself through: {loop}") + value = variables[name] + if isinstance(value, (list, dict)): + value = walk(value, chain + [name]) + else: + value = copy.deepcopy(value) + resolved[name] = value + return value + + def walk(node, chain): + if isinstance(node, str) and node.startswith("variable:"): + target = node.removeprefix("variable:") + if target not in variables: + available = ", ".join(sorted(variables.keys())) or "" + raise VariableNotFoundError( + f"Variable <{target}> not found; available variables: {available}" + ) + return resolve(target, chain) + if isinstance(node, list): + return [walk(item, chain) for item in node] + if isinstance(node, dict): + return {key: walk(item, chain) for key, item in node.items()} + return copy.deepcopy(node) + + for name in variables: + resolve(name, []) + return resolved +``` + +In `undeclared_variable_references`, replace the final loop so list/dict variable values are walked too: + +```python + for key, value in definition.items(): + if key != "variables": + walk(value, key) + if isinstance(declared, dict): + for name, value in declared.items(): + if isinstance(value, (list, dict)): + walk(value, f"variables.{name}") + return found +``` + +and update its docstring: "Walks everything but `variables` itself, the way resolution does" becomes "Walks everything but `variables` itself, plus the inside of every list- or dict-valued variable, the way `resolve_variable_values` and `replace_variables` together do." + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `pytest tests/test_variables.py -q` +Expected: PASS. + +- [ ] **Step 5: Write the failing workflow-level tests** + +Append to `tests/test_workflow.py` (it already has `_for_each_workflow(**overrides)` and `_workflow_from(definition, tmp_path)`; reuse them): + +```python +def test_an_entry_may_reference_another_variable(tmp_path): + """A shot entry's "from_file": "variable:voice" is the voice variable's + value by the time the member exists.""" + definition = _for_each_workflow() + definition["variables"]["voice"] = "cast/priya.wav" + definition["variables"]["shots"] = [ + {"name": "a", "text": "one", "voice": "variable:voice"} + ] + definition["steps"][0]["task"]["arguments"]["voice"] = "item:voice" + workflow = _workflow_from(definition, tmp_path) + + expanded = workflow.expanded_definition() + + assert expanded["steps"][0]["task"]["arguments"]["voice"] == "cast/priya.wav" + + +def test_an_undeclared_reference_inside_a_default_entry_is_a_validation_error( + tmp_path, +): + definition = _for_each_workflow() + definition["variables"]["shots"] = [{"name": "a", "text": "variable:nope"}] + workflow = _workflow_from(definition, tmp_path) + + errors = workflow.validation_errors() + + assert [e["path"] for e in errors] == ["variables.shots[0].text"] + assert "names no declared variable" in errors[0]["message"] + + +def test_an_undeclared_reference_inside_a_caller_s_entry_is_reported_under_arguments( + tmp_path, +): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a", "text": "variable:nope"}]} + ) + + assert [e["path"] for e in errors] == ["arguments.shots[0].text"] + + +def test_a_caller_s_entry_may_reference_a_declared_variable(tmp_path): + definition = _for_each_workflow() + definition["variables"]["voice"] = None + workflow = _workflow_from(definition, tmp_path) + + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a", "text": "variable:voice"}]} + ) + + assert errors == [] +``` + +Read `_for_each_workflow` first (`tests/test_workflow.py:622`) to confirm the step is a `task` step whose arguments include `"text": "item:text"`; if its field is named differently, use that name in place of `text` above. + +- [ ] **Step 6: Run them to verify they fail** + +Run: `pytest tests/test_workflow.py -q -k "entry"` +Expected: the first fails with the literal `variable:voice` string surviving; the undeclared ones fail with an empty error list or the wrong path. + +- [ ] **Step 7: Wire resolution into the workflow** + +In `dw/workflow.py`, import `resolve_variable_values` alongside the other `.variables` imports. + +`expanded_definition` — replace the substitution block: + +```python + definition = copy.deepcopy(self.workflow_definition) + variables = definition.get("variables") + if isinstance(variables, dict): + if arguments and not argument_errors(definition, arguments): + set_variables(arguments, variables) + variables = resolve_variable_values(variables) + definition = replace_variables(definition, variables) + return expand_for_each(definition, source_indices) +``` + +`validation_errors` — pass the arguments through: `return self._undeclared_variable_errors(arguments)`. + +`_undeclared_variable_errors` — take and fold the arguments: + +```python + def _undeclared_variable_errors(self, arguments=None): + """Every 'variable:' reference naming nothing the workflow declares. + + Fatal rather than a warning: once a workflow has a 'variables' + block, replace_variables refuses an undeclared reference, so this is + a run that cannot start. Good caller `arguments` are folded in first, + and a reference inside one of them is reported under `arguments.`, + where the caller wrote it. + """ + definition = copy.deepcopy(self.workflow_definition) + variables = definition.get("variables") + supplied = set() + if isinstance(variables, dict) and arguments: + if not argument_errors(definition, arguments): + set_variables(arguments, variables) + supplied = set(arguments) + declared = sorted(variables or {}) + + def where(path): + head, _, rest = path.partition(".") + if head == "variables": + name = rest.split(".", 1)[0].split("[", 1)[0] + if name in supplied: + return "arguments." + rest + return path + + return [ + { + "path": where(path), + "message": ( + f"'variable:{name}' names no declared variable; " + f"declared: {', '.join(declared) or ''}" + ), + } + for path, name in undeclared_variable_references(definition) + ] +``` + +`Workflow.run` — resolve before `realize_args`, so a `reference_type` inside an entry is a type name when the realizer reaches it: + +```python + set_variables(arguments, variables) + # an entry of a list-valued variable may name another + # variable; resolve those before anything inside it is + # realized, so a reference type in an entry is a type name + variables = resolve_variable_values(variables) + # realize the variables, initialiting downloads of images etc + realize_args(variables, base_dir) +``` + +Check the two later uses of `variables` in that function (the realized-workflow write and the seed) still read the resolved dict — `realize_workflow` takes `arguments` and re-folds them itself, so nothing else changes. Also open `dw/realize.py:75-93` and confirm it does not need resolution: it writes the variables as declared, with the run's arguments folded, and a nested `variable:` string there is right — a rerun resolves it again. + +- [ ] **Step 8: Run the tests and the full suite** + +Run: `pytest tests/test_workflow.py tests/test_variables.py -q` then `pytest -q -x tests/` +Expected: PASS; full suite green. + +- [ ] **Step 9: Commit** + +```bash +git add dw/variables.py dw/workflow.py tests/test_variables.py tests/test_workflow.py +git commit -m "feat(variables): an entry of a list-valued variable may reference another variable + +Resolved once before realize_args, reported as undeclared at the entry's +path, cycles refused. This is how a for_each template's optional voice +variable reaches every shot the character speaks in. + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 2: The catalog sees a `gather:` concat as a cut over a list + +**Files:** +- Modify: `dw/server/catalog_shape.py:160-183` (`_cuts_together`) +- Test: `tests/test_catalog_shape.py` + +**Interfaces:** +- Consumes: `derive_catalog_metadata(definition)`, the test file's `definition`, `pipeline_step`, `task_step` helpers. +- Produces: `_cuts_together` returns True for `"videos": "gather:"` exactly as it does for `"videos": "variable:"`. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_catalog_shape.py`, next to `test_a_concat_fed_by_two_steps_is_a_sequence`: + +```python +def test_a_concat_over_a_gathered_for_each_group_is_a_sequence(): + """A list-driven template's editor says "videos": "gather:shot" - a + cut over as many shots as the list holds, which the file cannot count.""" + meta = derive_catalog_metadata( + definition( + { + "name": "shot", + "for_each": "variable:shots", + "pipeline": { + "arguments": {"prompt": "item:prompt"}, + }, + "result": {"content_type": "video/mp4"}, + }, + task_step("cut", "concat_videos", {"videos": "gather:shot"}, "video/mp4"), + ) + ) + assert meta["shape"] == "sequence" +``` + +Read the file's `pipeline_step` helper first; if it can produce a step with a `for_each` key, use it instead of the literal dict, so the test reads like its neighbours. + +- [ ] **Step 2: Run it to verify it fails** + +Run: `pytest tests/test_catalog_shape.py -q -k gathered` +Expected: FAIL, shape is `shot`. + +- [ ] **Step 3: Recognise the prefix** + +In `_cuts_together`, change the string check and its docstring: + +```python + """A concat or dissolve fed by two or more distinct steps, or by a list + of shots handed in whole - one `variable:` reference is a supplied list + whose length only the caller knows, one `gather:` reference is every + member of a for_each group, and a cut over either is still an edit.""" + ... + if isinstance(videos, str) and videos.startswith(("variable:", "gather:")): + return True +``` + +- [ ] **Step 4: Run the catalog tests** + +Run: `pytest tests/test_catalog_shape.py tests/test_catalog_structure.py -q` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add dw/server/catalog_shape.py tests/test_catalog_shape.py +git commit -m "fix(catalog): a concat over gather: derives sequence + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 3: `music-video` on a `shots` list + +**Files:** +- Modify: `workflows/templates/minimax/music-video.json` +- Modify: `tests/test_for_each.py` (`TestMusicVideoTemplate`, lines 630-712) +- Modify: `workflows/templates/minimax/README.md:136` (the table row) + +**Interfaces:** +- Consumes: `expand_for_each`, `Workflow.expanded_definition()` (Task 1), `resolve_variable_values`. +- Produces: the template's `variables.shots`, a list of four `{name, prompt, start_frame}` entries; steps `draw_singer`, `write_song`, `slice` (for_each), `soundtrack`, `shot` (for_each), `edit`, `music_video`. + +- [ ] **Step 1: Rewrite the template** + +Start from the file as it is. Changes, in order: + +1. In `variables`: delete `shot_1_wide_open`, `shot_2_closeup`, `shot_3_room`, `shot_4_finale`. In their place (after `audio_duration`) add: + +```json + "shots": [ + { + "name": "wide_open", + "start_frame": 0, + "prompt": "" + }, + { + "name": "closeup", + "start_frame": 124, + "prompt": "" + }, + { + "name": "room", + "start_frame": 248, + "prompt": "" + }, + { + "name": "finale", + "start_frame": 372, + "prompt": "" + } + ], +``` + +Move each prompt string byte-for-byte (copy the JSON string literal, escapes and all); do not retype it. + +2. Replace the four `slice_N` steps with one: + +```json + { + "name": "slice", + "for_each": "variable:shots", + "task": { + "command": "slice_audio", + "arguments": { + "audio": "previous_result:write_song", + "sample_rate": "variable:sample_rate", + "start_frame": "item:start_frame", + "num_frames": "variable:num_frames", + "fps": "variable:fps" + } + } + }, +``` + +3. Keep `soundtrack` as it is. + +4. Replace `shot_1_wide_open` and the three `pipeline_reference` steps with one step: the full `shot_1_wide_open` step, renamed `shot`, with `"for_each": "variable:shots"` inserted directly after `"name"`, `"prompt": "item:prompt"`, and the audio reference's `"from_previous_result": "slice_1"` changed to `"from_previous_result": "slice"`. Everything else in the block (configuration, quantization, components, loras, `num_frames`, `width`, `height`, `num_inference_steps`, `output`, `result`) stays exactly as it was. + +5. In `edit`, replace the four-element `videos` list with `"videos": "gather:shot"`. + +6. Rewrite `description` (one string; keep the catalog `summary`-less form the file has — it has no `summary`, so the first sentence is the summary): + +``` +A music video built from cuts, sung to a soundtrack that never touches a chain. The long-take way to film a song - one chained generation lip-synced end to end - degrades with every carried segment and can let the sync slip. This builds the video the way music television does instead: MiniMax-Music3 writes the song, one 'slice_audio' step per entry of 'shots' cuts it into frame-exact pieces (124 frames at 24 fps each, from each entry's 'start_frame'), and one shot per entry is generated fresh from the same Z-Image portrait plus its own slice, lip-synced to just those five seconds. 'shots' is a list, so the cut is an argument: add an entry and there is one more slice and one more shot, named for it ('slice@closeup', 'shot@closeup'), and the loaded model is reused across every shot - identical pipeline definitions share one model. No shot conditions on another shot's output, so the last cut is as clean as the first. 'concat_videos' gathers the shots in list order, and because each one covered exactly its slice's frames, the edit is sample-accurate by construction: 'pair_audio' drops the original, unbroken song over the whole cut and the mouths line up in every shot. The generation models pass through one at a time - each is released before the next loads - so the workflow peaks no higher than its largest single model. +``` + +- [ ] **Step 2: Validate the file the way the CLI does** + +Run: `python -m dw.validate workflows/templates/minimax/music-video.json` +Expected: valid, no errors. Then `python -c "import json; json.load(open('workflows/templates/minimax/music-video.json'))"` to be sure the edit left valid JSON. + +- [ ] **Step 3: Replace `TestMusicVideoTemplate`** + +Delete the class at `tests/test_for_each.py:630-712` and write in its place: + +```python +class TestMusicVideoTemplate: + """music-video's slices and shots are two for_each groups over one + 'shots' list, paired by entry name: shot@closeup reads slice@closeup.""" + + KEYS = ["wide_open", "closeup", "room", "finale"] + + def expanded(self): + from dw.workflow import Workflow + + return Workflow(load_template("music-video.json"), TEMPLATES).expanded_definition() + + def test_the_template_validates_as_it_will_run(self): + from dw.workflow import Workflow + + assert Workflow(load_template("music-video.json"), TEMPLATES).validation_errors() == [] + + def test_one_slice_and_one_shot_per_entry_in_list_order(self): + names = [s["name"] for s in self.expanded()["steps"]] + assert names == ( + ["draw_singer", "write_song"] + + [f"slice@{k}" for k in self.KEYS] + + ["soundtrack"] + + [f"shot@{k}" for k in self.KEYS] + + ["edit", "music_video"] + ) + + def test_each_slice_starts_where_its_entry_says(self): + got = steps_by_name(self.expanded()) + starts = [got[f"slice@{k}"]["task"]["arguments"]["start_frame"] for k in self.KEYS] + assert starts == [0, 124, 248, 372] + + def test_each_shot_reads_its_own_slice_and_the_one_portrait(self): + got = steps_by_name(self.expanded()) + for key in self.KEYS: + references = got[f"shot@{key}"]["pipeline"]["arguments"]["references"] + assert [r["from_previous_result"] for r in references] == [ + "draw_singer", + f"slice@{key}", + ] + + def test_each_shot_carries_its_entry_s_prompt(self): + template = load_template("music-video.json") + got = steps_by_name(self.expanded()) + for entry in template["variables"]["shots"]: + prompt = got[f"shot@{entry['name']}"]["pipeline"]["arguments"]["prompt"] + assert prompt == entry["prompt"] + assert prompt.startswith("subject_definitions:") + + def test_every_shot_is_the_same_pipeline(self): + """Full pipeline blocks rather than pipeline_reference: the identity + cache reuses the loaded model, so this costs no reload.""" + from dw.workflow import pipeline_cache_key + + got = steps_by_name(self.expanded()) + keys = {pipeline_cache_key(got[f"shot@{k}"]["pipeline"]) for k in self.KEYS} + assert len(keys) == 1 + + def test_the_edit_gathers_the_shots_in_order(self): + got = steps_by_name(self.expanded()) + assert got["edit"]["task"]["arguments"]["videos"] == [ + f"previous_result:shot@{k}" for k in self.KEYS + ] + + def test_the_realized_file_keeps_the_list(self): + """workflow.json beside a run is the source form: 'for_each' and the + 'shots' variable, not the expanded members.""" + template = load_template("music-video.json") + assert "shots" in template["variables"] + assert [s["name"] for s in template["steps"] if "for_each" in s] == ["slice", "shot"] +``` + +Check `Workflow.__init__`'s signature (`dw/workflow.py`, `class Workflow`) before writing: if it takes `(workflow_definition, base_dir, ...)` in a different order or by keyword, match it. If the file's `without_pipeline_reference` helper is now unused by any test, delete it. + +- [ ] **Step 4: Run the tests** + +Run: `pytest tests/test_for_each.py tests/test_examples.py tests/test_catalog_structure.py tests/test_plugin_skills.py -q` +Expected: PASS. If `test_example_type_references_resolve` complains about a `*_type` inside `shots`, read its skip rules (`tests/test_examples.py:135-160`) — a `variable:`-prefixed value is skipped; the music-video entries carry no `*_type` keys, so this should not fire. + +- [ ] **Step 5: Update the README row** + +`workflows/templates/minimax/README.md:136` becomes: + +``` +| [music-video.json](music-video.json) | A music video cut to a generated song, one `shots` list driving both `for_each` groups: `slice_audio` deals each entry its frame-exact piece, the shot lip-syncs to it, and `pair_audio` lays the unbroken track over the finished edit | +``` + +- [ ] **Step 6: Full suite and commit** + +Run: `pytest -q -x tests/` +Expected: green. + +```bash +git add workflows/templates/minimax/music-video.json tests/test_for_each.py workflows/templates/minimax/README.md +git commit -m "feat(templates): music-video's slices and shots are two for_each groups over one shots list + +Breaking for scripted callers: shot_1_wide_open .. shot_4_finale are +entries of 'shots' now, each {name, prompt, start_frame}. + +Co-Authored-By: Claude Fable 5.1 " +``` + +--- + +### Task 4: `dialogue-short` on a `shots` list + +**Files:** +- Modify: `workflows/templates/minimax/dialogue-short.json` +- Modify: `tests/test_dialogue_short_voices.py` +- Modify: `tests/test_for_each.py` (`TestDialogueShortTemplate`, the class after `TestMusicVideoTemplate`) +- Modify: `workflows/templates/minimax/README.md:135` +- Modify: `tests/test_catalog_structure.py` only if a description-mentions test fails (see Step 4) + +**Interfaces:** +- Consumes: Task 1's nested resolution (entries say `"from_file": "variable:character_a_voice"` and `"reference_type": "variable:subject_reference_type"`), `Workflow.expanded_definition()`, `realize_args`. +- Produces: `variables.shots`, five `{name, prompt, references, num_frames}` entries; steps `draw_character_a`, `draw_character_b`, `shot` (for_each), `episode`. The variables `shot_1_cold_open` … `shot_5_tag`, `num_frames` and `tag_num_frames` are gone; `character_a_voice` / `character_b_voice`, the two `*_reference_type` variables and everything else stay. + +- [ ] **Step 1: Rewrite the template** + +1. In `variables`, delete `shot_1_cold_open` … `shot_5_tag`, `num_frames` and `tag_num_frames`. After `character_b_portrait_prompt` add `shots`. Each entry's `references` is the reference list its hand-written step carried, verbatim; each `prompt` is the string moved byte-for-byte from the deleted variable: + +```json + "shots": [ + { + "name": "cold_open", + "num_frames": 124, + "references": [ + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_a"}, + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_b"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_a_voice"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_b_voice"} + ], + "prompt": "" + }, + { + "name": "deflect", + "num_frames": 124, + "references": [ + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_b"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_b_voice"} + ], + "prompt": "" + }, + { + "name": "react", + "num_frames": 124, + "references": [ + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_a"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_a_voice"} + ], + "prompt": "" + }, + { + "name": "button", + "num_frames": 124, + "references": [ + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_b"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_b_voice"} + ], + "prompt": "" + }, + { + "name": "tag", + "num_frames": 141, + "references": [ + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_a"}, + {"reference_type": "variable:subject_reference_type", "from_previous_result": "draw_character_b"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_a_voice"}, + {"reference_type": "variable:voice_reference_type", "from_file": "variable:character_b_voice"} + ], + "prompt": "" + } + ], +``` + +Format the reference objects one key per line like the rest of the file (four-space indent); the compact form above is for reading. + +2. Replace `shot_1_cold_open` and the four `pipeline_reference` steps with one step: the full `shot_1_cold_open` block renamed `shot`, `"for_each": "variable:shots"` after `"name"`, and in `arguments`: `"prompt": "item:prompt"`, `"references": "item:references"`, `"num_frames": "item:num_frames"`. Everything else unchanged. + +3. In `episode`, `"videos": "gather:shot"`. + +4. Rewrite `description`: + +``` +A digital short built the way television is built: from cuts, not from one long take. Chained generation degrades with length - every segment conditions on the previous segment's output, so artifacts compound and identity drifts. A scene cut resets that completely: each shot here is generated fresh from the same two character portraits, so shot five is exactly as clean as shot one and the scene can run as long as the script does. Two Z-Image steps draw the cast (the second reuses the first's loaded pipeline - identical configurations share one model - and 'release_pipeline' frees it before the video model loads). The shots are one 'for_each' step over the 'shots' list: one entry per shot, carrying its 'name', its 'prompt', its 'references' and its 'num_frames' - the tag entry runs 141 frames where the others run 124, since length is per-shot. The list is an argument, so a six-shot scene is one more entry, not another file, and the members are named for their entries ('shot@react'); the loaded MiniMax-H3 is reused across all of them. Character consistency across cuts comes from referencing the same portraits in every shot; voice consistency comes from repeating each character's voice description verbatim in every prompt, and, when 'character_a_voice' / 'character_b_voice' name a clip ('asset:cast/priya.wav'), from the audio reference each entry lists for whoever speaks in it - an entry's reference says 'variable:character_a_voice', so one variable sets the voice in every shot that character has. Both default to null, and a reference whose file is null is left out of the list, so a run that names no voice generates exactly what it generated before the variables existed. A shot where both speak lists both; if that reads worse than one, name one voice and leave the other null. The variables are named for roles rather than for the cast of this example - 'character_a', the 'react' entry - because the beats are the reusable part and the sketch is not. The soundscape writes the laugh track. A final 'concat_videos' task is the editor, gathering the shots in list order into one episode - hard cuts, no trims, no seams to hide, because nothing was carried between them. Only the picture cuts hard: 'audio_bleed_ms' rings each shot's laugh track on over the silent opening of the next, the way a live audience carries across a cut. It and 'seam_fade_ms' are variables, so a seam is re-tuned with an argument rather than a copy of the workflow; 1800 ms is where a five-shot cut measured best, since a generated shot opens on more silence than it looks. +``` + +Keep `summary` as it is. + +- [ ] **Step 2: Validate** + +Run: `python -m dw.validate workflows/templates/minimax/dialogue-short.json` +Expected: valid. + +- [ ] **Step 3: Rewrite the voices test** + +Replace the body of `tests/test_dialogue_short_voices.py` from `SPEAKERS` down (keep the module docstring, add one sentence to it: "The shots are entries of a list now, and an entry's voice reference names the variable, so the same one variable still sets the voice everywhere.") with: + +```python +# Which character speaks in which shot - each entry lists a voice +# reference per speaker, and this is the mapping it is asserted against +SPEAKERS = { + "shot@cold_open": ("a", "b"), + "shot@deflect": ("b",), + "shot@react": ("a",), + "shot@button": ("b",), + "shot@tag": ("a", "b"), +} + + +@pytest.fixture +def definition(): + with open(TEMPLATE, encoding="utf-8") as file: + return json.load(file) + + +def shot_references(definition, arguments): + """Every shot member's references, as the engine realizes them for a + run: the caller's arguments folded, entries resolved, the list expanded, + then each member's arguments realized as its step would.""" + from dw.workflow import Workflow + + expanded = Workflow(definition, os.path.dirname(TEMPLATE)).expanded_definition( + arguments + ) + references = {} + for step in expanded["steps"]: + if step["name"] not in SPEAKERS: + continue + arguments = step["pipeline"]["arguments"] + realize_args(arguments) + references[step["name"]] = arguments["references"] + assert set(references) == set(SPEAKERS) + return references + + +class TestOptionalVoices: + def test_the_voices_default_to_null(self, definition): + variables = definition["variables"] + assert variables["character_a_voice"] is None + assert variables["character_b_voice"] is None + + def test_every_entry_names_the_voice_variables_rather_than_a_file(self, definition): + for entry in definition["variables"]["shots"]: + voices = [ + r["from_file"] for r in entry["references"] if "from_file" in r + ] + assert voices, entry["name"] + assert all(v.startswith("variable:character_") for v in voices), entry["name"] + + def test_no_voice_named_leaves_only_the_portraits(self, definition): + for name, references in shot_references(definition, {}).items(): + assert len(references) == len(set(SPEAKERS[name])), name + assert all( + reference["from_previous_result"].startswith("draw_character") + for reference in references + ), name + + def test_a_named_voice_is_referenced_in_the_shots_it_speaks_in( + self, definition, tmp_path + ): + from tests.test_media_info import write_wav + + voice = tmp_path / "priya.wav" + write_wav(voice, seconds=1.0) + + for name, references in shot_references( + definition, {"character_a_voice": str(voice)} + ).items(): + built = [r for r in references if not isinstance(r, dict)] + assert len(built) == (1 if "a" in SPEAKERS[name] else 0), name + + def test_the_tag_runs_longer(self, definition): + frames = {e["name"]: e["num_frames"] for e in definition["variables"]["shots"]} + assert frames == { + "cold_open": 124, "deflect": 124, "react": 124, "button": 124, "tag": 141 + } + + def test_the_variable_names_are_roles_rather_than_a_cast(self, definition): + """Every run carried howie_portrait_prompt and shot_3_howie_incredulous + through its arguments, manifest and export whatever the cast was.""" + names = " ".join(definition["variables"]) + " ".join( + step["name"] for step in definition["steps"] + ) + " ".join(e["name"] for e in definition["variables"]["shots"]) + assert "howie" not in names.lower() + assert "pat_" not in names.lower() + assert "character_a_portrait_prompt" in definition["variables"] + assert [e["name"] for e in definition["variables"]["shots"]] == [ + "cold_open", "deflect", "react", "button", "tag" + ] +``` + +Note the realized `from_file` reference is built at variable-realization time in a real run (before expansion) and at member-realization time here; both go through `realize_object`, and a built object passes through `realize_args` unchanged, so the count is the same either way. If `realize_args` in `shot_references` fails on an already-loaded `reference_type` (a `type`, not a string), that is a bug to report as a concern, not to work around. + +- [ ] **Step 4: Replace `TestDialogueShortTemplate`** + +```python +class TestDialogueShortTemplate: + """dialogue-short's five shots are one for_each group whose entries + carry everything that differs between shots: prompt, references and + length.""" + + KEYS = ["cold_open", "deflect", "react", "button", "tag"] + + def expanded(self): + from dw.workflow import Workflow + + return Workflow(load_template("dialogue-short.json"), TEMPLATES).expanded_definition() + + def test_the_template_validates_as_it_will_run(self): + from dw.workflow import Workflow + + assert Workflow(load_template("dialogue-short.json"), TEMPLATES).validation_errors() == [] + + def test_one_shot_per_entry_between_the_cast_and_the_edit(self): + names = [s["name"] for s in self.expanded()["steps"]] + assert names == ( + ["draw_character_a", "draw_character_b"] + + [f"shot@{k}" for k in self.KEYS] + + ["episode"] + ) + + def test_each_shot_references_the_portraits_its_entry_lists(self): + got = steps_by_name(self.expanded()) + portraits = { + key: [ + r["from_previous_result"] + for r in got[f"shot@{key}"]["pipeline"]["arguments"]["references"] + if "from_previous_result" in r + ] + for key in self.KEYS + } + assert portraits == { + "cold_open": ["draw_character_a", "draw_character_b"], + "deflect": ["draw_character_b"], + "react": ["draw_character_a"], + "button": ["draw_character_b"], + "tag": ["draw_character_a", "draw_character_b"], + } + + def test_the_reference_types_are_resolved_inside_the_entries(self): + """An entry's "variable:subject_reference_type" is the dotted type + name by the time the member exists - never the literal reference.""" + got = steps_by_name(self.expanded()) + for key in self.KEYS: + for r in got[f"shot@{key}"]["pipeline"]["arguments"]["references"]: + assert r["reference_type"].startswith("diffusers.modular_pipelines") + + def test_the_tag_runs_longer(self): + got = steps_by_name(self.expanded()) + frames = [got[f"shot@{k}"]["pipeline"]["arguments"]["num_frames"] for k in self.KEYS] + assert frames == [124, 124, 124, 124, 141] + + def test_every_shot_is_the_same_pipeline(self): + from dw.workflow import pipeline_cache_key + + got = steps_by_name(self.expanded()) + assert len({pipeline_cache_key(got[f"shot@{k}"]["pipeline"]) for k in self.KEYS}) == 1 + + def test_the_episode_gathers_the_shots_in_order(self): + got = steps_by_name(self.expanded()) + assert got["episode"]["task"]["arguments"]["videos"] == [ + f"previous_result:shot@{k}" for k in self.KEYS + ] +``` + +- [ ] **Step 5: Run the affected tests** + +Run: `pytest tests/test_for_each.py tests/test_dialogue_short_voices.py tests/test_examples.py tests/test_catalog_structure.py tests/test_plugin_skills.py -q` +Expected: PASS. If `test_a_description_names_only_variables_the_workflow_declares` fails because the description quotes `'shots'`-entry words that another workflow declares as a variable, read its allow-list at `tests/test_catalog_structure.py:210` and either reword the description or extend the list with a comment saying why; do not weaken the test. + +- [ ] **Step 6: Update the README row** + +`workflows/templates/minimax/README.md:135`: + +``` +| [dialogue-short.json](dialogue-short.json) | A five-shot sitcom scene: Z-Image draws the cast, one `for_each` step over a `shots` list generates a shot per entry - its prompt, its references, its length - on one loaded model, and `concat_videos` gathers the episode | +``` + +- [ ] **Step 7: Full suite and commit** + +Run: `pytest -q -x tests/` + +```bash +git add workflows/templates/minimax/dialogue-short.json tests/test_dialogue_short_voices.py tests/test_for_each.py workflows/templates/minimax/README.md tests/test_catalog_structure.py +git commit -m "feat(templates): dialogue-short's five shots are one for_each group over a shots list + +Breaking for scripted callers: shot_1_cold_open .. shot_5_tag, num_frames +and tag_num_frames are entries of 'shots' now, each {name, prompt, +references, num_frames}. An entry's voice reference names +character_a_voice / character_b_voice, so one variable still sets a voice +in every shot the character speaks in. + +Co-Authored-By: Claude Fable 5.1 " +``` + +(Drop `tests/test_catalog_structure.py` from `git add` if it was not touched.) + +--- + +### Task 5: The skill, the guide, CLAUDE.md and the proposal say so + +**Files:** +- Modify: `plugins/dw/skills/minimax-h3/SKILL.md` (the "A piece with cuts" bullet; size cap 12288 bytes) +- Modify: `tests/test_plugin_skills.py` (`TestMiniMaxH3Skill`) +- Modify: `docs/WORKFLOW_GUIDE.md` (the `### One step per entry: for_each` section, ~line 340-410) +- Modify: `CLAUDE.md` (the `for_each` bullet in Type System, ~line 157; the `for_each` gotcha in Critical Gotchas) +- Modify: `docs/proposals/list-driven-steps.md` (status line, lines 3-5; "Notes for stage 2") + +**Interfaces:** +- Consumes: the templates' new `shots` entry shapes from Tasks 3 and 4. +- Produces: skill text an agent reads before composing; tests that pin it. + +- [ ] **Step 1: Write the failing skill tests** + +Add to `TestMiniMaxH3Skill` in `tests/test_plugin_skills.py`: + +```python + def test_the_cuts_templates_are_described_as_list_driven(self): + """Both cut templates take one 'shots' list; the skill says what an + entry carries, so an agent writes entries rather than the shot_N_* + arguments T005 and this rewrite removed.""" + import json + + text = skill_text(H3_SKILL) + assert "`shots`" in text + assert "shot_1_" not in text and "shot_2_" not in text + for name, fields in ( + ("dialogue-short", {"name", "prompt", "references", "num_frames"}), + ("music-video", {"name", "prompt", "start_frame"}), + ): + path = os.path.join( + REPO_ROOT, "workflows", "templates", "minimax", name + ".json" + ) + spec = json.load(open(path, encoding="utf-8")) + entries = spec["variables"]["shots"] + assert all(set(entry) == fields for entry in entries), name + for field in fields: + assert f"`{field}`" in text, f"the skill does not name {field}" + # cost scales with the list, and the listing's figure is the default's + assert "per shot" in text or "per entry" in text +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `pytest tests/test_plugin_skills.py -q -k list_driven` +Expected: FAIL on `` `shots` ``. + +- [ ] **Step 3: Edit the skill** + +In `plugins/dw/skills/minimax-h3/SKILL.md`, the bullet beginning `- **A piece with cuts**:`. Replace its first two sentences + +``` + `templates/minimax/dialogue-short` (Z-Image draws the cast, one loaded model + per shot, `concat_videos` splices) and `templates/minimax/music-video` + (shots cut to a generated song, lip-synced slices). A cut erases drift; the + last shot is as clean as the first. Write shots, not takes. +``` + +with + +``` + `templates/minimax/dialogue-short` (Z-Image draws the cast, one shot per + entry of its `shots` list on one loaded model, `concat_videos` splices) and + `templates/minimax/music-video` (a song, one slice and one lip-synced shot + per entry). `shots` is one list argument: a dialogue entry is `name`, + `prompt`, `references` (which portraits and voices this shot uses) and + `num_frames`; a music-video entry is `name`, `prompt` and `start_frame`. + A six-shot piece is one more entry, not another file; the listing's `cost` + is the default list's, so quote it per shot times the entries you write. + A cut erases drift; the last shot is as clean as the first. Write shots, + not takes. +``` + +and later in the same bullet, change + +``` + Each shot's `num_frames` is its own, so pace the + cut - a trailer builds by varying shot length. +``` + +to + +``` + Each entry's `num_frames` is its own, so pace the cut. +``` + +Then measure: `wc -c plugins/dw/skills/minimax-h3/SKILL.md`. If over 12288, tighten elsewhere in the same bullet (the Bark/narrator sentences are the longest and can lose "Bark's presets are conversational;") until under. Do not cut a hard rule or a source. + +- [ ] **Step 4: Run the plugin tests** + +Run: `pytest tests/test_plugin_skills.py -q` +Expected: PASS, including the size cap. + +- [ ] **Step 5: The guide** + +In `docs/WORKFLOW_GUIDE.md`, in `### One step per entry: for_each`, after the paragraph beginning "`item:field` is the whole value of that field", add: + +``` +An entry may name another variable: `"from_file": "variable:character_a_voice"` +inside a `references` entry is that variable's value by the time the member +exists, so one variable sets a voice in every shot the character speaks in +and a caller who supplies the list still writes `variable:` for the parts the +template fixes. Those references are resolved before anything in the entry is +loaded, and an undeclared one is a validation error at the entry's path +(`arguments.shots[2].references[1].from_file` when the list is yours, +`variables.shots[...]` when it is the template's). A value may not reference +itself, directly or through another variable. +``` + +And in the closing "Limits" paragraph, after "quote `cost × len(list)`", add nothing new (it already says it), but change "`validate_workflow` with the `arguments` you will run with: it expands your list, not the template's default" to "`validate_workflow` with the `arguments` you will run with: it expands your list, not the template's default, resolves the variables your entries name". Then at the end of the section, before `### The loop`, add: + +``` +`templates/minimax/dialogue-short` and `templates/minimax/music-video` are +this shape: each takes one `shots` list, and `get_workflow` on either shows +the entry an item needs. +``` + +- [ ] **Step 6: CLAUDE.md** + +In the Type System `for_each` bullet, after "two groups over the same list pair by key (`slice` inside `shot@x` is `slice@x`)." insert: "An entry of a list-valued variable may reference another variable (`"from_file": "variable:character_a_voice"`); `resolve_variable_values` (`dw/variables.py`) replaces those once, before `realize_args`, refusing a cycle, and `undeclared_variable_references` walks inside list/dict variable values too." + +In Critical Gotchas, add a bullet after the `for_each` one: + +``` +- **The two MiniMax cut templates take one `shots` list** — since the stage-2 + rewrite (2026-09-11) `templates/minimax/dialogue-short` and `music-video` + have no `shot_N_*` variables; a scripted caller passes `shots` (entries + `{name, prompt, references, num_frames}` and `{name, prompt, start_frame}`). + The members are `shot@` in the manifest and the gallery. This is the + breaking change the next release note should name +``` + +- [ ] **Step 7: The proposal** + +`docs/proposals/list-driven-steps.md` lines 3-5 become: + +``` +Status: **stages 1 and 2 implemented** (expansion pass, validation, schema, +docs - `dw/for_each.py`; `music-video` and `dialogue-short` on a `shots` +list, 2026-09-11); stage 3 (catalog per-entry cost and entry shape) not +started. Written for MCP feedback ticket T003. +``` + +Under "## Notes for stage 2", add a closing paragraph: + +``` +Resolved in stage 2: the `pipeline_reference` question went away - every +member is the full pipeline block, and the identity-keyed pipeline cache +reuses the loaded model exactly as the reference did (a test holds every +member to one `pipeline_cache_key`). A group's `pipeline_reference` still +gets no directed error; nothing bundled uses one now. Entries that name +other variables (`variable:character_a_voice`) are resolved by +`resolve_variable_values` before `realize_args`, which is what let the +optional voices move into the entries. The empty-list and +`realize_constants` questions are still open and belong to stage 3 with the +catalog work. +``` + +- [ ] **Step 8: Full suite, then commit** + +Run: `pytest -q -x tests/` + +```bash +git add plugins/dw/skills/minimax-h3/SKILL.md tests/test_plugin_skills.py docs/WORKFLOW_GUIDE.md CLAUDE.md docs/proposals/list-driven-steps.md +git commit -m "docs(for_each): the cut templates take one shots list; entries may name variables + +Co-Authored-By: Claude Fable 5.1 " +``` From 29e8b6bb074a7d9b5cb7edbca348b200e09657ed Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:25:07 -0500 Subject: [PATCH 13/38] fix(mcp): T020 reference check at submission, T021 progress inside a generation T020 - run_workflow refused an argument *name* it did not recognise and queued a bad 'asset:' reference anyway, so a typo came back as a job id and a status: running, then died on the first step. The name half of the check lives in JobManager.submit, where the definition is loaded; the reference half needs the workspace's search path, so POST /api/jobs now makes the same _argument_reference_errors call POST /api/validate does. Same message from both, and nothing is queued. T021 - a ModularPipeline (H3, LTX-2, Qwen-Image) has no callback_on_step_end, so the route that reports every other pipeline's denoise steps never fired: one 'generating' phase and then nothing for the whole loop, which reads exactly like a hung run. Its denoise block drives a tqdm bar instead, so reported_progress_bars patches that bar per instance for the length of the call - every advance is a pipeline_step event and a cancellation checkpoint, and the patch is undone on the way out. Uses _blocks, not the public blocks, which hands back a deepcopy. Beside it, a running job carries a `progress` block on GET /api/jobs/{id} and on the MCP poll: the step, the phase and how long it has been in it, seconds_since_event, and the denoise counter once that loop runs - kept as events arrive rather than derived from a log that gets trimmed. That is what separates a slow run from a stuck one between two otherwise identical polls. T016 - proposal only, per Don: docs/proposals/output-folders.md. Additive throughout; no field renamed or removed. --- docs/MCP.md | 14 +- docs/SERVER.md | 18 +- docs/proposals/output-folders.md | 268 +++++++++++++++++++++++++++++ dw/pipeline_processors/pipeline.py | 166 +++++++++++++++++- dw/server/app.py | 17 ++ dw/server/jobs.py | 68 ++++++++ dw_mcp/diagnose.py | 20 ++- dw_mcp/server.py | 16 +- tests/test_job_progress.py | 114 ++++++++++++ tests/test_modular_progress.py | 173 +++++++++++++++++++ tests/test_validate_arguments.py | 56 ++++++ 11 files changed, 913 insertions(+), 17 deletions(-) create mode 100644 docs/proposals/output-folders.md create mode 100644 tests/test_job_progress.py create mode 100644 tests/test_modular_progress.py diff --git a/docs/MCP.md b/docs/MCP.md index 8b7cc315..208d8c94 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -283,11 +283,11 @@ references written in the same session. | Tool | Arguments | Purpose | | --- | --- | --- | | `run_workflow(workflow_path=None, inline_workflow=None, arguments=None, acknowledged_cost=False, workspace=None)` | exactly one of `workflow_path` (a catalog name from `list_workflows`, with or without `.json`, or a path to a workflow file on the server) or `inline_workflow`, optional `arguments`, `acknowledged_cost`, `workspace` | Queue a workflow for generation. Returns as soon as the job is queued. `workspace` names the workspace for this one call without switching the session to it - use it to pin a job whose `output:` or `asset:` references live in a workspace other than the session's | -| `get_job(job_id)` | `job_id` | Get a job's status, warnings, output manifest, error and traceback | +| `get_job(job_id)` | `job_id` | Get a job's status, warnings, output manifest, error and traceback. A running job also carries `progress` (below) | | `get_job_workflow(job_id)` | `job_id` | The workflow the job actually ran. `realized: true` means every mutable input is pinned (arguments, seed, prompts, `output:latest`); `false` means the job predates run tracking and this is the definition as submitted. Pass it to `save_workflow` to keep it under a name | | `export_job(job_id, overwrite=False)` | `job_id`, `overwrite` | Gather one finished job into `/exports//` on the server: the realized workflow, the run's manifest, the job row, a README, and copies of the assets, earlier-run inputs and outputs. Returns the directory, a zip URL, the file list with sizes and the total. The three JSON files are in the zip, not repeated here - get_job_workflow and get_job serve them individually. **The directory is on the machine running the server**, like `download_output`'s destination - fetch the zip URL and unpack it into `exports/` under the session's working directory (a deliverable, not a temp file); the archive already unpacks into one folder named after the job id | | `get_job_events(job_id, after=-1, limit=200)` | `job_id`, `after`, `limit` | Get a page of a job's progress events | -| `wait_for_job(job_id, timeout_seconds=20)` | `job_id`, `timeout_seconds` | Block until a job reaches a terminal status, or `timeout_seconds` elapses. **One call blocks for at most 55 seconds** — a larger `timeout_seconds` is clamped, not honoured, because no MCP client holds a tool call open for a generation's real runtime, so budget one call per ~55s of the job. Every reply carries `waited_seconds`, `timeout_requested_seconds`, `timeout_applied_seconds` and `timeout_capped`, so a capped return is distinguishable from an elapsed one. Use instead of hand-polling `get_job`/`get_job_events` in a loop; if it returns `still_running: true`, call it again. Returns a slim job - status, warnings, error, and the manifest once finished - without the arguments; `get_job` has those | +| `wait_for_job(job_id, timeout_seconds=20)` | `job_id`, `timeout_seconds` | Block until a job reaches a terminal status, or `timeout_seconds` elapses. **One call blocks for at most 55 seconds** — a larger `timeout_seconds` is clamped, not honoured, because no MCP client holds a tool call open for a generation's real runtime, so budget one call per ~55s of the job. Every reply carries `waited_seconds`, `timeout_requested_seconds`, `timeout_applied_seconds` and `timeout_capped`, so a capped return is distinguishable from an elapsed one. Use instead of hand-polling `get_job`/`get_job_events` in a loop; if it returns `still_running: true`, call it again. Returns a slim job - status, warnings, error, and the manifest once finished - without the arguments; `get_job` has those. A running job also carries `progress` (below) | | `cancel_job(job_id)` | `job_id` | Ask a queued or running job to stop | | `rerun_job(job_id, acknowledged_cost=False, new_seed=False)` | `job_id`, `acknowledged_cost`, `new_seed` | Queue a fresh job from a previous job's stored specification. Costs GPU time, so it passes the same gate as `run_workflow`. `new_seed=true` draws a fresh seed into the workflow's seed variable — without it a seeded workflow's rerun repeats its arguments exactly and the step cache serves the whole run from the earlier one's files (`reused: true`), generating nothing. `get_job_workflow`'s `seed_variable` says whether there is one | | `move_job(job_id, direction)` | `job_id`, `direction` (`up`\|`down`\|`front`\|`back`) | Reorder a queued job | @@ -358,6 +358,16 @@ The intended loop: if it failed) 5. `get_output_image(name)` to look at a result image +While a job runs, `get_job` and `wait_for_job` carry a `progress` block - +the step being run, the phase (`loading`, `generating`, `decoding`, +`saving`) with the model in `phase_detail`, `seconds_in_phase`, +`seconds_since_event`, and `denoise_step`/`denoise_total_steps` once the +denoise loop starts. A single-step generation is minutes of one phase, so +two polls otherwise come back identical: read `denoise_step` moving (slow +but healthy) against `seconds_since_event` climbing with nothing else +changing (nothing is happening). `cancel_job` stops at the next denoise or +step boundary, which `denoise_step` is also the measure of. + ## Security The MCP server adds no authentication of its own — it inherits the REST diff --git a/docs/SERVER.md b/docs/SERVER.md index e9097f2b..4046196d 100644 --- a/docs/SERVER.md +++ b/docs/SERVER.md @@ -164,7 +164,7 @@ Every event in the stream carries a `seq` and an `event` name: | `workflow_start` | the run begins | `workflow`, `total_steps`, `steps`, `seed` | | `step_start` / `step_end` | each step | `step`, `index`, `total_steps`; `files` at the end. A step served from the step cache adds `reused: true` to `step_end`, and its `files` are the earlier run's files rather than newly written ones | | `iteration_start` | each argument combination in a step | `step`, `iteration`, `total_iterations` | -| `pipeline_step` | each denoise step | `step`, `total_steps` | +| `pipeline_step` | each denoise step | `step`, `total_steps`. Emitted for a pipeline that takes a `callback_on_step_end`, and for a `ModularPipeline` (H3, LTX-2, Qwen-Image), which takes none - there the denoise block's own progress bar is what reports | | `phase` | the step changes what it is doing | `phase`, `detail` | | `workflow_end` | the run finishes | `manifest` | @@ -177,6 +177,22 @@ restarts), `decoding` (latents, after the last denoise step), `saving` (writing files, including video encode) and `task` (a task step, named in `detail`). Emits are a handful per step, not per denoise tick. +### Progress on a running job + +`GET /api/jobs/{id}` carries a `progress` block while a job is running +(`null` before it starts and once it is terminal, where the manifest is the +better answer). It is the same information the event log holds, kept as the +events arrive so a caller does not have to page back through a trimmed log +to learn where a long render is: + +| field | | +| --- | --- | +| `step`, `step_index`, `total_steps` | the workflow step being run | +| `phase`, `phase_detail` | the latest phase and what it named | +| `seconds_in_phase` | how long it has been in it | +| `seconds_since_event` | how long since anything at all happened - the number that separates a slow run from a stuck one | +| `denoise_step`, `denoise_total_steps` | present once the denoise loop is running | + ## Introspection API The editor's forms come from these; they are just as usable from scripts: diff --git a/docs/proposals/output-folders.md b/docs/proposals/output-folders.md new file mode 100644 index 00000000..b080168a --- /dev/null +++ b/docs/proposals/output-folders.md @@ -0,0 +1,268 @@ +# Proposal: folders for a job's outputs + +Status: **proposed, not implemented**. Written for MCP feedback ticket T016. + +## The ask, as filed + +> Jobs and workspace outputs support folders one level deep - the same +> shape, look, and behaviour as the folder support that already exists for +> the workflow and prompt libraries. The intent is separation of +> intermediate outputs from final ones. Folders carry through all three +> surfaces: MCP tools, REST API, and UI. MCP consumers are steered toward +> using them for the intermediate/final distinction. +> +> Actual: everything a job produces sits at one level, so a finished episode +> is indistinguishable from the twenty scratch files that went into it +> without reading names. + +The report is accurate. A `dialogue-short` run writes its five shot videos, +its portrait stills, its sliced audio beds and its one assembled episode +into a single run directory, distinguished only by the step name embedded in +each file name. The consumer of that run - an agent deciding what to show +the user, or a person opening the gallery - has to know the workflow to know +which file is the deliverable. + +## Why this is a proposal and not an edit + +A destination folder is a new field in the workflow schema, which means it +is a concept every consumer has to learn and every surface has to carry. But +the reason it is a proposal rather than an edit is narrower than that: **the +output tree already has a layer with the meaning this wants.** + +A run writes to `///`, and three +separate things read that shape by position: + +- `strip_run_id` (`dw/runs.py`) turns `ltx2/Gyre/20260905-181530-a1b2c3d4/ + still-0.png` into the gallery folder `ltx2/Gyre`, which is how a workflow + run fifty times is one entry in the folder filter rather than fifty. +- `output:` references are `//`, with + `latest` allowed in the run-id position (`resolve_output_reference`). +- The manifest, and `workflow.json` beside it, sit at the root of the run + directory and describe everything in it. + +Adding a folder *inside* a run directory puts a segment between the run id +and the file name, and every one of those three readers has to be told which +segment is which. Getting that wrong is not a cosmetic bug: `strip_run_id` +would start reporting `ltx2/Gyre/final` and `ltx2/Gyre/intermediate` as two +unrelated workflows in the gallery filter, and an `output:` reference that +worked yesterday would resolve to nothing. That is the part that needs +deciding before anything is written. + +There is also a question the ticket settles one way and the existing code +settles the other, covered under [Open questions](#open-questions): the +workflow and prompt libraries do not actually enforce one level. + +## The shape + +A step's `result` block names the folder its files are written into: + +```json +{ + "name": "assemble_episode", + "task": { "command": "concat_videos", "arguments": { "...": "..." } }, + "result": { + "content_type": "video/mp4", + "folder": "final" + } +} +``` + +- `folder` is optional. Without it, files are written where they are written + today: at the root of the run directory. **Every existing workflow keeps + working unchanged, and a workflow that never sets `folder` is + indistinguishable from one written before this existed.** +- The value is a relative path validated the way every other + workflow-supplied path segment is - through `validate_output_path` against + the run directory, so `../` and absolute paths are refused rather than + escaping into another run. +- It may carry a `variable:` reference, so a caller can route a run's + outputs without editing the workflow. +- It is per step, not per workflow: the point is that one run's steps land + in different places. + +The convention the tooling steers toward is two names, `final` and +`intermediate` - see [Steering](#steering-consumers-toward-it). The engine +does not know those names or treat them specially; a workflow that wants +`shots/` and `audio/` gets them. + +## What each surface does with it + +### The run directory and the manifest + +``` +outputs/dialogue-short/20260911-205805-a1b2c3d4/ + manifest.json + workflow.json + intermediate/dialogue_short-shot_a.0-0.0.mp4 + intermediate/dialogue_short-slice_a.0-0.0.wav + final/dialogue_short-assemble.0-0.0.mp4 +``` + +The manifest already records each step's files as paths relative to the run +directory (`manifest_relative_files`), so a foldered file appears as +`final/dialogue_short-assemble.0-0.0.mp4` with no schema change to the +manifest at all. Each manifest entry additionally carries `folder` (the +empty string for an unfoldered step), so a consumer can group without +parsing paths. + +### `get_job` and `list_jobs` + +`get_job`'s manifest gains the `folder` on each entry, which is additive. On +top of that, a finished job's reply carries: + +```json +"outputs": { "final": ["final/...mp4"], "intermediate": ["intermediate/...mp4", "..."] } +``` + +- a grouping of the manifest's files by folder, so "what did this produce" + is answerable without walking the manifest. A job whose workflow uses no + folders reports `{"": [...]}`, which is the honest answer rather than a + guess at which file is the deliverable. + +`list_jobs` stays a listing: it gains nothing per job, because a row that +carried file lists is what T018 just finished cutting down. The `total`/ +`truncated` shape is untouched. + +### The gallery listing (`GET /api/gallery`, MCP `list_gallery`) + +This is the surface where the decision above bites. The rule: + +- `folder` on a gallery entry stays **the workflow identity**, exactly as + today - `ltx2/Gyre`, not `ltx2/Gyre/final`. The folder filter continues to + group a workflow's runs, which is what it is for. +- A new field, `group` (`final`, `intermediate`, or `""`), carries the + in-run folder, and `GET /api/gallery` takes an optional `?group=` filter + beside the existing `?folder=`. +- `strip_run_id` is extended to drop everything after the run id rather than + just the file name: `ltx2/Gyre//final/x.mp4` -> `ltx2/Gyre`, and the + discarded segments become the `group`. Today's paths are unaffected + because they have nothing after the run id. + +This keeps the two axes separate - *which workflow* and *which part of the +run* - rather than multiplying them into one filter list that grows by a +factor of two for every workflow. + +### `asset:` and `previous_result:` + +- **`previous_result:` is unaffected.** It names a step, and results are + passed in memory between steps; where a step's files landed on disk has + never been part of it. A foldered step is read back exactly as it is now. +- **`output:` gains one segment.** The name is + `//`, where `` is already allowed + to be a relative path - `_resolve_segments` walks segments and + `validate_output_reference` permits `/`. So + `output:dialogue-short/latest/final/episode.mp4` works with **no change to + the resolver**, and the `latest` search (newest run that holds the file) + keeps working because it is a per-run existence check on the whole + remainder of the path. +- **`asset:` is unaffected.** The asset library is a separate tree and + already takes nested names (`asset:gyre/frames/web.mp4`). +- **`keep_output`** takes the output's relative name, which now may contain + the folder, and writes into the asset library under `asset_name` as + before. The output's folder is not copied into the asset name - the two + trees mean different things, and a promoted file is by definition final. + +### The web UI + +One change, mapped onto what the gallery already has: beside the existing +folder filter (workflow identity), a second, smaller control for `group`, +defaulting to showing everything. A run that used no folders shows exactly +what it shows today. The job page groups the manifest it already renders +under the same headings. + +The editor's form for a `result` block gains `folder` the way it gains every +other schema field - from the schema, with no editor-specific work. + +### Breaking changes + +**None over MCP or REST.** Everything above is additive: a new optional +workflow field, new keys on existing replies, a new optional query +parameter. Two things a scripted consumer should know: + +1. A manifest file name may now contain a `/` where it previously could not. + Anything that assumed a manifest entry was a bare file name - splitting + on `/` and taking the last part, or joining it to a directory by hand - + keeps working, but a consumer that wants the folder should read the + `folder` field rather than parsing the path. +2. `gallery` entries gain `group`; `folder` keeps its current meaning. A + consumer filtering on `folder` sees no change. + +## Steering consumers toward it + +The ticket is explicit that this only pays off if the MCP consumer uses it, +and a schema field nobody sets is worth nothing. Four places, in the order +an agent meets them: + +1. **The templates.** Every multi-step template in `workflows/templates/` + marks its deliverable step `"folder": "final"` and its scratch steps + `"folder": "intermediate"`. This is the one that matters most: agents + compose by copying a template, so the convention propagates whether or + not anyone reads a description. +2. **The authoring guide** (`docs/WORKFLOW_GUIDE.md`, the `Authoring a + workflow from an agent` section, and its CLAUDE.md mirror) states the + two-name convention and the rule: a step whose output the user will be + shown is `final`, everything else is `intermediate`. +3. **Tool descriptions.** `run_workflow` says nothing new (it does not + author). `get_job` names the `outputs` grouping, and `save_workflow`'s + description states the convention for a workflow being stored for reuse. +4. **The plugin skills** (`plugins/dw/skills/*`) each state it for their + family's templates, which is where a composing agent is already reading. + +## What this does not do + +- **No nesting policy for the libraries.** The workflow, prompt and asset + libraries are untouched. +- **No automatic classification.** The engine does not guess which step is + final. A workflow that says nothing gets today's behaviour, and the + reports say `""` rather than inventing an answer. +- **No retention policy.** "Prune the intermediates, keep the finals" is an + obvious next thing to want and is not part of this: it needs a decision + about what happens to an `output:` reference pointing into a pruned + folder, and that is its own proposal. +- **No move-after-the-fact.** There is no "mark this output final" call on a + finished job. The folder is decided by the workflow, at write time. +- **No change to file naming.** Names stay + `{workflow}-{step}.{i}-{j}.{k}.ext`; the folder is a prefix, not a + replacement for the step name in the name. + +## Phasing + +1. **The engine.** `folder` in the schema and in `Result.save`, path + validation against the run directory, `folder` on manifest entries, + `strip_run_id` extended. Tests: an unfoldered workflow writes exactly + what it writes today; a foldered one lands where it says; `../` is + refused; `output:.../latest/final/x.mp4` resolves. +2. **The server surfaces.** `outputs` grouping on `get_job`, `group` on + gallery entries, `?group=` filter, MCP pass-through, docs. +3. **The steering.** Templates marked up, guide and skills updated, the + catalog re-audited so `list_workflows` traits still hold. +4. **The UI.** The group control on the gallery, grouped manifest on the job + page. + +Stages 1 and 2 are the ticket; 3 is what makes it used; 4 is what makes it +visible to a person rather than an agent. + +## Open questions + +1. **One level, or a path?** The ticket asks for one level deep, "to stay + consistent with the workflow and prompt libraries". Those libraries are + in fact not one level deep - both walk the tree (`workflow_names`, + the prompt library's `prompt:folder/name`) and a name is a relative path + of any depth; `templates/minimax/reference-to-video` is a real catalog + name with two levels. So consistency with them means *not* enforcing a + depth limit. My recommendation: allow a path, validate it, and make the + *convention* one level (`final`, `intermediate`) in the templates and the + guide - enforcement would be a rule with nothing behind it, and it would + block `shots/act-1` on a workflow that wants it. This is the one place + where the proposal as written diverges from the ticket, and it is Don's + call. +2. **Should `get_job`'s `outputs` grouping exist at all**, or is the + `folder` field on each manifest entry enough? The grouping is + redundant-but-cheap; it exists because it answers "what did this + produce" in one read, which is the question the ticket is actually + about. +3. **`final` as a default for a one-step workflow?** A `shot`-shape + workflow has exactly one output and it is the deliverable. Defaulting it + to `final` would make the common case right for free - but it would also + mean the same workflow's files move on upgrade, which breaks any stored + `output:` reference to them. Recommendation: no default, ever. diff --git a/dw/pipeline_processors/pipeline.py b/dw/pipeline_processors/pipeline.py index d7ec53e3..8aeb29ae 100644 --- a/dw/pipeline_processors/pipeline.py +++ b/dw/pipeline_processors/pipeline.py @@ -506,23 +506,35 @@ def _call_pipeline(self, arguments, attn_backend): stack.enter_context(attention_backend(attn_backend)) stack.enter_context(stateful_cache_context(self.pipeline)) + if not self._takes_step_callback(): + # A modular pipeline takes no step callback at all, so this + # is the only per-step signal it has: the denoise blocks + # drive a tqdm bar, and a bar that reports each advance is + # the difference between a slow run and a hung one + stack.enter_context(reported_progress_bars(self.pipeline)) return self.pipeline(**arguments) + def _takes_step_callback(self): + """Whether this pipeline names `callback_on_step_end` in its own + signature. Only a pipeline that names the parameter explicitly gets + one - a **kwargs signature is no promise the pipeline honors it, and + a ModularPipeline (H3, LTX-2, Qwen-Image) has no such parameter at + all, which is why it needs the progress-bar route instead.""" + try: + parameters = inspect.signature(self.pipeline.__call__).parameters + except (TypeError, ValueError): + return False + return "callback_on_step_end" in parameters + def _with_step_callback(self, arguments): """Inject a callback_on_step_end that reports per-step progress to the active run context and raises when the run has been cancelled. Workflow JSON cannot express a callable, so this is the only way a - diffusion-step callback ever reaches a pipeline call. Only pipelines - that name the parameter explicitly get one - a **kwargs signature is - no promise the pipeline honors it. + diffusion-step callback ever reaches a pipeline call. """ - try: - parameters = inspect.signature(self.pipeline.__call__).parameters - except (TypeError, ValueError): - return arguments - if "callback_on_step_end" not in parameters: + if not self._takes_step_callback(): return arguments run_context = get_context() @@ -1750,6 +1762,144 @@ def get_cache_transformer(pipeline): return None +class _ReportingProgressBar: + """A tqdm bar that also reports each advance to the active run. + + Wraps rather than subclasses, because the bar it wraps is whatever the + block's own progress_bar() built - tqdm, or a notebook bar, or whatever + a future diffusers uses. Everything it does not intercept falls through + to the real bar, so the terminal output is unchanged. + """ + + def __init__(self, bar, on_advance, total=None): + self._bar = bar + self._on_advance = on_advance + # Counted here rather than read off the bar: a disabled tqdm - which + # is what a quiet server or a notebook config leaves you with - keeps + # its own `n` at zero while still being advanced normally + self._done = 0 + self._total = total if total is not None else getattr(bar, "total", None) + + def update(self, n=1): + result = self._bar.update(n) + self._done += n or 0 + self._on_advance(self._done, self._total) + return result + + def __iter__(self): + # Reported after the body of the loop has run, not before it: the + # step is finished when control comes back here + for item in self._bar: + yield item + self._done += 1 + self._on_advance(self._done, self._total) + + def __enter__(self): + self._bar.__enter__() + return self + + def __exit__(self, *exception): + return self._bar.__exit__(*exception) + + def __getattr__(self, name): + # Guarded: the wrapped bar is the first thing __init__ sets, and an + # unguarded lookup of it before then recurses forever + if name == "_bar": + raise AttributeError(name) + return getattr(self._bar, name) + + +def _progress_bar_holders(pipeline): + """Every object under a modular pipeline that can open a progress bar. + + The denoise loop is a block, not the pipeline, and it calls its own + `self.progress_bar(...)` - so the tree is what has to be walked. Uses + `_blocks`, not the public `blocks`, which hands back a deepcopy: patching + a copy would report nothing and look like this never worked. + """ + holders = [] + seen = set() + + def walk(candidate): + if candidate is None or id(candidate) in seen: + return + seen.add(id(candidate)) + # __dict__, because the patch is an instance attribute: an object + # with none could not be patched and must not be tried + if callable(getattr(candidate, "progress_bar", None)) and hasattr( + candidate, "__dict__" + ): + holders.append(candidate) + children = getattr(candidate, "sub_blocks", None) + if hasattr(children, "values"): + for child in children.values(): + walk(child) + + walk(pipeline) + walk(getattr(pipeline, "_blocks", None)) + return holders + + +@contextlib.contextmanager +def reported_progress_bars(pipeline): + """Report each denoise step of a pipeline that takes no step callback. + + A ModularPipeline - H3, LTX-2, Qwen-Image and every family diffusers has + moved over - has no `callback_on_step_end` parameter, so the whole + denoise loop passed in silence: one 'generating' phase, then nothing for + however many minutes it took, which reads exactly like a hung run. What + those blocks do have is a tqdm bar, and every advance of it is a step. + + The patch is per-instance and undone on the way out, so a pipeline this + process keeps loaded is handed back as it was found. + """ + holders = _progress_bar_holders(pipeline) + if not holders: + yield + return + + run_context = get_context() + + def on_advance(done, total): + run_context.emit("pipeline_step", step=done, total_steps=total) + # Past the last step there is still the decode, which on video is + # minutes with the bar sitting at 100% + if done is not None and total is not None and done >= total: + emit_phase("decoding") + # The one cancellation checkpoint inside a modular denoise loop: + # without it a cancel waits out the whole generation + run_context.check_cancelled() + + patched = [] + for holder in holders: + original = holder.progress_bar + # Whether the name was already an attribute of the instance decides + # how it is put back: restored, or removed so the class method shows + # through again rather than a bound copy of it being frozen on + patched.append((holder, original, "progress_bar" in vars(holder))) + + def reporting(iterable=None, total=None, _original=original): + bar = _original(iterable=iterable, total=total) + if total is None and iterable is not None: + # An iterated bar's total is the length of what it iterates, + # when that can be known at all + total = getattr(bar, "total", None) + return _ReportingProgressBar(bar, on_advance, total) + + holder.progress_bar = reporting + try: + yield + finally: + for holder, original, was_own in patched: + if was_own: + holder.progress_bar = original + else: + try: + del holder.progress_bar + except AttributeError: + holder.progress_bar = original + + @contextlib.contextmanager def stateful_cache_context(pipeline): """Provide the context a stateful cache hook reads its state through. diff --git a/dw/server/app.py b/dw/server/app.py index 256d94ea..9d9ade51 100644 --- a/dw/server/app.py +++ b/dw/server/app.py @@ -813,6 +813,23 @@ def submit_job(request: JobRequest, ws: Workspace = Depends(selected_workspace)) resolved, source = resolve_workflow_reference( request.workflow_path, sources ) + # The same reference check POST /api/validate makes, because a + # caller who skipped the free pre-flight should still not get a + # job id for an argument that cannot resolve. The name half of + # this check lives in JobManager.submit, where the definition is + # loaded; this half needs the workspace's search path, which is + # here - which is why a bad 'asset:' used to queue and die on the + # first step while a bad variable name was refused outright + reference_problems = _argument_reference_errors( + request.arguments, workspace + ) + if reference_problems: + raise ValueError( + "; ".join( + f"{problem['path']}: {problem['message']}" + for problem in reference_problems + ) + ) job = manager.submit( workflow_path=resolved, workflow=request.workflow, diff --git a/dw/server/jobs.py b/dw/server/jobs.py index e58cd0bf..cbf908bc 100644 --- a/dw/server/jobs.py +++ b/dw/server/jobs.py @@ -351,13 +351,80 @@ def __init__(self, spec): self.run_id = None self.run_dir = None self.events = [] + # The running summary a poll reads - see _note_progress. Kept as the + # events arrive rather than derived from the log on request, because + # the log is trimmed to its last MAX_PERSISTED_EVENTS and a caller + # polling a long render should not have to page through it to learn + # that something moved + self.last_event_at = None + self.phase = None + self.phase_detail = None + self.phase_started_at = None + self.step_name = None + self.step_index = None + self.total_steps = None + self.denoise_step = None + self.denoise_total_steps = None self.condition = threading.Condition() def add_event(self, event): with self.condition: self.events.append({"seq": len(self.events), **event}) + self._note_progress(event) self.condition.notify_all() + def _note_progress(self, event): + """Fold one event into the running summary. + + A single-step generation emits `generating` and then nothing until it + is done, so 'no new events' is the normal state of a healthy run and + says nothing about whether it is progressing. What answers that is + how long it has been that way, and how far into the denoise loop it + got - both of which are here rather than in the event log. + """ + now = time.time() + self.last_event_at = now + kind = event.get("event") + if kind == "phase": + self.phase = event.get("phase") + self.phase_detail = event.get("detail") + self.phase_started_at = now + elif kind == "pipeline_step": + self.denoise_step = event.get("step") + self.denoise_total_steps = event.get("total_steps") + elif kind == "step_start": + self.step_name = event.get("step") + self.step_index = event.get("index") + self.total_steps = event.get("total_steps") + # A new step's denoise loop has not started; the previous step's + # count would read as this one's progress + self.denoise_step = None + self.denoise_total_steps = None + + def progress(self): + """Where a running job has got to, or None for one that has not + started or has finished - a terminal job has a manifest, which is a + better answer than a stale phase.""" + if self.status != RUNNING or self.last_event_at is None: + return None + now = time.time() + summary = { + "step": self.step_name, + "step_index": self.step_index, + "total_steps": self.total_steps, + "phase": self.phase, + "phase_detail": self.phase_detail, + "seconds_in_phase": round(now - self.phase_started_at, 1) + if self.phase_started_at + else None, + # The one number that separates a slow run from a hung one + "seconds_since_event": round(now - self.last_event_at, 1), + } + if self.denoise_step is not None: + summary["denoise_step"] = self.denoise_step + summary["denoise_total_steps"] = self.denoise_total_steps + return summary + def finish(self, status, error=None, traceback_text=None): self.status = status self.finished_at = time.time() @@ -406,6 +473,7 @@ def detail(self): "traceback": self.traceback, "event_count": len(self.events), "run_dir": self.run_dir, + "progress": self.progress(), } diff --git a/dw_mcp/diagnose.py b/dw_mcp/diagnose.py index 4dcffaa4..5f1a1cf5 100644 --- a/dw_mcp/diagnose.py +++ b/dw_mcp/diagnose.py @@ -72,7 +72,9 @@ def run_workflow( def get_job(client, job_id): - """A job's status, arguments, warnings, manifest, error and traceback.""" + """A job's status, arguments, warnings, manifest, error and traceback. + A running job also carries `progress` - the step, the phase and how long + it has been in it, with the denoise counter once that loop starts.""" return client.get_json(api_path("api", "jobs", job_id)) @@ -118,6 +120,12 @@ def get_job_events(client, job_id, after=-1, limit=200): "warnings", "error", "event_count", + # Where a running job has got to: the step, the phase and how long it + # has been in it, plus the denoise counter when one is running. A + # single-step generation emits nothing for minutes at a time, so this + # is what separates a slow job from a hung one on a poll that would + # otherwise come back byte-identical + "progress", ) @@ -152,7 +160,15 @@ def wait_for_job(client, job_id, timeout_seconds=20): returns the job's last-seen status with `still_running: true` instead of hanging - call again to keep waiting. Returns a slim job - status, warnings, error, and the manifest once finished - without the - arguments; get_job has those.""" + arguments; get_job has those. + + A running job carries `progress`: the step it is on, the phase + (`loading`, `generating`, `decoding`, `saving`) with the model or step + named in `phase_detail`, `seconds_in_phase`, `seconds_since_event`, and + `denoise_step`/`denoise_total_steps` once the denoise loop is running. + Two calls with the same phase and a growing `seconds_in_phase` but a + moving `denoise_step` is a slow run; one where nothing moves and + `seconds_since_event` keeps climbing is a stuck one.""" requested = max(0.0, float(timeout_seconds)) applied = min(requested, float(MAX_WAIT_SECONDS)) capped = applied < requested diff --git a/dw_mcp/server.py b/dw_mcp/server.py index 729b162b..65d5c116 100644 --- a/dw_mcp/server.py +++ b/dw_mcp/server.py @@ -705,9 +705,11 @@ def get_job_workflow(job_id: str) -> dict: return diagnose.get_job_workflow(client, job_id) def get_job_events(job_id: str, after: int = -1, limit: int = 200) -> dict: - """Get a page of a job's progress events - phase transitions, memory - readings and log lines. `after` is exclusive: pass back the previous - call's `last_seq` to continue.""" + """Get a page of a job's progress events - phase transitions, denoise + steps, memory readings and log lines. `after` is exclusive: pass back + the previous call's `last_seq` to continue. For 'is it still moving?' + the `progress` block on get_job/wait_for_job is cheaper than a page + of events.""" return diagnose.get_job_events(client, job_id, after=after, limit=limit) def wait_for_job(job_id: str, timeout_seconds: int = 20) -> dict: @@ -726,7 +728,13 @@ def wait_for_job(job_id: str, timeout_seconds: int = 20) -> dict: timeout_capped. Returns a slim job - status, warnings, error, and the manifest once - finished - without the arguments; get_job has those.""" + finished - without the arguments; get_job has those. A running job + also carries `progress`: the step it is on, the phase (`loading`, + `generating`, `decoding`, `saving`) with the model named in + `phase_detail`, `seconds_in_phase`, `seconds_since_event`, and + `denoise_step`/`denoise_total_steps` once the denoise loop starts - + which is how a slow run and a stuck one tell apart between two + otherwise identical polls.""" return diagnose.wait_for_job(client, job_id, timeout_seconds=timeout_seconds) # The cap is a number a caller paces against, so the description states diff --git a/tests/test_job_progress.py b/tests/test_job_progress.py new file mode 100644 index 00000000..c9c1cd4b --- /dev/null +++ b/tests/test_job_progress.py @@ -0,0 +1,114 @@ +"""What a poll learns about a job that is still running. + +A single-step generation - every `shot`-shape template, the most expensive +thing the server does - emits `generating` and then nothing until it is +finished. Polls come back byte-identical for minutes, so "no new events" is +the normal state of a healthy run and cannot be read as trouble. The +progress block is what a caller reads instead: the step, the phase, how long +it has been in it, and the denoise counter once that loop is running. +""" + +from dw.server.jobs import Job, RUNNING, QUEUED, SUCCEEDED +from dw_mcp.diagnose import slim_job + + +def running_job(*events): + job = Job({"workflow_name": "shot"}) + job.status = RUNNING + for event in events: + job.add_event(event) + return job + + +def test_a_job_that_has_not_started_reports_no_progress(): + job = Job({"workflow_name": "shot"}) + job.status = QUEUED + + assert job.progress() is None + assert job.detail()["progress"] is None + + +def test_a_finished_job_reports_no_progress(): + """Its manifest is a better answer than a phase it has left.""" + job = running_job({"event": "phase", "phase": "generating"}) + job.finish(SUCCEEDED) + + assert job.progress() is None + + +def test_the_step_and_phase_are_reported(): + job = running_job( + {"event": "step_start", "step": "shot_a", "index": 0, "total_steps": 2}, + {"event": "phase", "phase": "loading", "detail": "MiniMaxAI/MiniMax-H3"}, + ) + + progress = job.progress() + assert progress["step"] == "shot_a" + assert progress["step_index"] == 0 and progress["total_steps"] == 2 + assert progress["phase"] == "loading" + assert progress["phase_detail"] == "MiniMaxAI/MiniMax-H3" + assert progress["seconds_in_phase"] >= 0 + assert progress["seconds_since_event"] >= 0 + + +def test_the_denoise_counter_appears_once_the_loop_runs(): + job = running_job({"event": "phase", "phase": "generating", "detail": "h3"}) + + assert "denoise_step" not in job.progress() + + job.add_event({"event": "pipeline_step", "step": 3, "total_steps": 20}) + + progress = job.progress() + assert progress["denoise_step"] == 3 + assert progress["denoise_total_steps"] == 20 + + +def test_a_new_step_drops_the_previous_step_counter(): + """Otherwise the last step's '20 of 20' reads as this one's progress.""" + job = running_job( + {"event": "step_start", "step": "a", "index": 0, "total_steps": 2}, + {"event": "pipeline_step", "step": 20, "total_steps": 20}, + {"event": "step_start", "step": "b", "index": 1, "total_steps": 2}, + ) + + progress = job.progress() + assert progress["step"] == "b" + assert "denoise_step" not in progress + + +def test_the_phase_clock_restarts_with_the_phase_but_the_event_clock_does_not(): + job = running_job({"event": "phase", "phase": "loading"}) + job.phase_started_at -= 30 + job.last_event_at -= 30 + + assert job.progress()["seconds_in_phase"] >= 30 + + job.add_event({"event": "phase", "phase": "generating"}) + + progress = job.progress() + assert progress["phase"] == "generating" + assert progress["seconds_in_phase"] < 1 + assert progress["seconds_since_event"] < 1 + + +def test_progress_survives_the_trim_of_the_event_log(): + """The log keeps its tail; the summary is kept as events arrive, so a + long render's phase is not something a caller has to page back for.""" + job = running_job({"event": "phase", "phase": "generating", "detail": "h3"}) + for seq in range(500): + job.add_event({"event": "log", "message": f"line {seq}"}) + + assert job.progress()["phase"] == "generating" + + +def test_the_mcp_poll_carries_it(): + job = running_job( + {"event": "step_start", "step": "shot_a", "index": 0, "total_steps": 1}, + {"event": "phase", "phase": "generating", "detail": "h3"}, + {"event": "pipeline_step", "step": 7, "total_steps": 20}, + ) + + slim = slim_job(job.detail()) + + assert slim["progress"]["denoise_step"] == 7 + assert slim["progress"]["phase"] == "generating" diff --git a/tests/test_modular_progress.py b/tests/test_modular_progress.py new file mode 100644 index 00000000..700ba18a --- /dev/null +++ b/tests/test_modular_progress.py @@ -0,0 +1,173 @@ +"""Per-step progress from a pipeline that takes no step callback. + +A ModularPipeline - H3, LTX-2, Qwen-Image - has no `callback_on_step_end` +parameter, so the route that reports every other pipeline's denoise steps +never fired for one: a `generating` phase, then nothing at all for however +many minutes the loop took. From outside, a slow run and a hung one were the +same thing. What those pipelines do have is a tqdm bar inside the denoise +block, and every advance of it is a step. +""" + +import copy + +import pytest +from PIL import Image +from tqdm.auto import tqdm + +from dw.events import RunContext, WorkflowCancelled + +from .test_phase_events import _pipeline_workflow, _run + + +class FakeOutput: + def __init__(self): + self.images = [Image.new("RGB", (2, 2))] + + +class FakeDenoiseBlock: + """The block that owns the loop, the way a modular denoise block does: + it opens its own bar and advances it once per step.""" + + def __init__(self, steps=3, iterate=False): + self.steps = steps + self.iterate = iterate + + def progress_bar(self, iterable=None, total=None): + if iterable is not None: + return tqdm(iterable, disable=True) + return tqdm(total=total, disable=True) + + def run(self): + if self.iterate: + for _ in self.progress_bar(range(self.steps)): + pass + return + with self.progress_bar(total=self.steps) as bar: + for _ in range(self.steps): + bar.update() + + +class FakeBlocks: + def __init__(self, denoise): + self.sub_blocks = {"denoise": denoise} + + +class FakeModularPipeline: + """No `callback_on_step_end` in the signature, and a public `blocks` + that hands back a copy - both true of the real thing, and the second is + why the patch has to go through `_blocks`.""" + + def __init__(self, steps=3, iterate=False): + self.denoise = FakeDenoiseBlock(steps, iterate) + self._blocks = FakeBlocks(self.denoise) + + @property + def blocks(self): + return copy.deepcopy(self._blocks) + + def __call__(self, prompt=None, num_inference_steps=None, generator=None): + self.denoise.run() + return FakeOutput() + + +class FakePipelineWithBoth: + """A classic pipeline: it has a bar *and* takes a callback. Only one of + the two may report, or every step is counted twice.""" + + def __init__(self): + self._num_timesteps = 3 + + def progress_bar(self, iterable=None, total=None): + return tqdm(total=total, disable=True) + + def __call__( + self, + prompt=None, + num_inference_steps=None, + generator=None, + callback_on_step_end=None, + ): + with self.progress_bar(total=3) as bar: + for i in range(3): + callback_on_step_end(self, i, 0, {}) + bar.update() + return FakeOutput() + + +def _events(fake, on_event=None): + collected = [] + + def sink(event): + collected.append(event) + if on_event is not None: + on_event(context, event) + + context = RunContext(on_event=sink) + _run(_pipeline_workflow(), context, fake=fake) + return collected + + +def _steps(events): + return [ + (event["step"], event["total_steps"]) + for event in events + if event["event"] == "pipeline_step" + ] + + +def test_every_advance_of_the_bar_is_reported(): + assert _steps(_events(FakeModularPipeline())) == [(1, 3), (2, 3), (3, 3)] + + +def test_an_iterated_bar_reports_too(): + """The other shape a block writes the loop in.""" + assert _steps(_events(FakeModularPipeline(iterate=True))) == [ + (1, 3), + (2, 3), + (3, 3), + ] + + +def test_decoding_follows_the_last_step(): + events = _events(FakeModularPipeline()) + + names = [ + event.get("phase") if event["event"] == "phase" else event["event"] + for event in events + if event["event"] in ("phase", "pipeline_step") + ] + assert names == [ + "loading", + "generating", + "pipeline_step", + "pipeline_step", + "pipeline_step", + "decoding", + ] + + +def test_a_cancel_lands_inside_the_loop_rather_than_after_it(): + """The same checkpoint the callback route has: without it a cancel on a + modular pipeline waits out the whole generation.""" + + def cancel_on_first_step(context, event): + if event["event"] == "pipeline_step": + context.cancel() + + with pytest.raises(WorkflowCancelled): + _events(FakeModularPipeline(steps=20), on_event=cancel_on_first_step) + + +def test_a_pipeline_that_takes_a_callback_is_not_counted_twice(): + assert _steps(_events(FakePipelineWithBoth())) == [(1, 3), (2, 3), (3, 3)] + + +def test_the_pipeline_is_handed_back_unpatched(): + """This process keeps a loaded pipeline between runs - a wrapper left on + it would report into the run that has finished.""" + fake = FakeModularPipeline() + + _events(fake) + + assert "progress_bar" not in vars(fake.denoise) + assert isinstance(fake.denoise.progress_bar(total=1), tqdm) diff --git a/tests/test_validate_arguments.py b/tests/test_validate_arguments.py index 2dbfe0a6..985a9dd3 100644 --- a/tests/test_validate_arguments.py +++ b/tests/test_validate_arguments.py @@ -222,3 +222,59 @@ def test_good_arguments_still_queue(self, server): ) assert response.status_code == 201 + + def test_a_bad_reference_is_refused_before_the_job_is_queued(self, server): + """The name half of the check was refused at submission and the + reference half was not, so a typo'd asset came back as a job id and + died on the first step - a success-shaped answer to a question + validate could answer for free.""" + with server() as client: + response = client.post( + "/api/jobs", + json={ + "workflow_path": "Typed", + "arguments": {"image": "asset:iirs.png"}, + }, + ) + + assert response.status_code == 400 + detail = response.json()["detail"] + assert "arguments.image" in detail and "iirs.png" in detail + assert client.get("/api/jobs").json()["jobs"] == [] + + def test_a_reference_that_resolves_still_queues(self, server): + with server() as client: + response = client.post( + "/api/jobs", + json={ + "workflow_path": "Typed", + "arguments": {"image": "asset:iris.png"}, + }, + ) + + assert response.status_code == 201 + + def test_an_inline_workflow_is_checked_the_same_way(self, server): + with server() as client: + response = client.post( + "/api/jobs", + json={ + "workflow": typed_workflow(), + "arguments": {"image": "asset:nowhere.png"}, + }, + ) + + assert response.status_code == 400 + assert "arguments.image" in response.json()["detail"] + + def test_submission_and_validation_give_the_same_message(self, server): + """The ticket was a consistency gap, not a missing check: what the + free pre-flight says is what submission says.""" + arguments = {"image": "asset:iirs.png"} + with server() as client: + validated = validate(client, arguments=arguments) + refused = client.post( + "/api/jobs", json={"workflow_path": "Typed", "arguments": arguments} + ) + + assert validated["errors"][0]["message"] in refused.json()["detail"] From e68bbf54b2cb07cc02d3eecc8d01ea52668b1d35 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:26:57 -0500 Subject: [PATCH 14/38] feat(variables): an entry of a list-valued variable may reference another variable Resolved once before realize_args, reported as undeclared at the entry's path, cycles refused. This is how a for_each template's optional voice variable reaches every shot the character speaks in. Co-Authored-By: Claude Fable 5.1 --- dw/variables.py | 70 +++++++++++++++++++++++++++++--- dw/workflow.py | 36 ++++++++++++++--- tests/test_variables.py | 89 ++++++++++++++++++++++++++++++++++++++++- tests/test_workflow.py | 53 ++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 12 deletions(-) diff --git a/dw/variables.py b/dw/variables.py index 4ac42e1d..f774c7ea 100644 --- a/dw/variables.py +++ b/dw/variables.py @@ -86,16 +86,72 @@ def replace_variables(data, variables): return copy.deepcopy(data) +def resolve_variable_values(variables): + """A copy of `variables` in which every "variable:name" inside a list- + or dict-valued variable is replaced by that variable's value. + + A list-driven step reads its entries from a variable, and an entry that + says "from_file": "variable:character_a_voice" is how one variable sets + a voice in every shot the character speaks in. replace_variables only + walks the definition, so those references would reach the step as the + literal strings; this resolves them once, before realize_args, so a + reference type inside an entry is a type name by the time it is loaded. + + Only list and dict values are walked. A scalar value that begins with + "variable:" is passed through as it always was. + + Raises: + VariableNotFoundError: a reference names nothing declared + ValueError: a value references itself, directly or through others + """ + resolved = {} + + def resolve(name, chain): + if name in resolved: + return resolved[name] + if name in chain: + loop = " -> ".join(chain[chain.index(name) :] + [name]) + raise ValueError(f"Variable '{name}' references itself through: {loop}") + value = variables[name] + if isinstance(value, (list, dict)): + value = walk(value, chain + [name]) + else: + value = copy.deepcopy(value) + resolved[name] = value + return value + + def walk(node, chain): + if isinstance(node, str) and node.startswith("variable:"): + target = node.removeprefix("variable:") + if target not in variables: + available = ", ".join(sorted(variables.keys())) or "" + raise VariableNotFoundError( + f"Variable <{target}> not found; available variables: {available}" + ) + return resolve(target, chain) + if isinstance(node, list): + return [walk(item, chain) for item in node] + if isinstance(node, dict): + return {key: walk(item, chain) for key, item in node.items()} + return copy.deepcopy(node) + + for name in variables: + resolve(name, []) + return resolved + + def undeclared_variable_references(definition): """The "variable:name" references in a workflow definition that name no entry of its `variables` - the ones `replace_variables` will refuse at run time, found before anything loads. - Walks everything but `variables` itself, the way resolution does. Returns - a list of (path, name) pairs, path being where the reference sits - (`steps[0].pipeline.arguments.prompt`) and name what it asked for - which - is the whole remainder of the string, since a reference is the entire - value and nothing is interpolated around it. + Walks everything but `variables` itself, plus the inside of every list- + or dict-valued variable, the way `resolve_variable_values` and + `replace_variables` together do. Returns a list of (path, name) pairs, + path being where the reference sits (`steps[0].pipeline.arguments.prompt`) + and name what it asked for - which is the whole remainder of the string, + since a reference is the entire value and nothing is interpolated + around it. """ declared = definition.get("variables") or {} found = [] @@ -115,6 +171,10 @@ def walk(node, path): for key, value in definition.items(): if key != "variables": walk(value, key) + if isinstance(declared, dict): + for name, value in declared.items(): + if isinstance(value, (list, dict)): + walk(value, f"variables.{name}") return found diff --git a/dw/workflow.py b/dw/workflow.py index 0ce358c4..44a34457 100644 --- a/dw/workflow.py +++ b/dw/workflow.py @@ -46,6 +46,7 @@ from .variables import ( argument_errors, replace_variables, + resolve_variable_values, set_variables, undeclared_variable_references, VariableNotFoundError, @@ -308,6 +309,7 @@ def expanded_definition(self, arguments=None, source_indices=None): if isinstance(variables, dict): if arguments and not argument_errors(definition, arguments): set_variables(arguments, variables) + variables = resolve_variable_values(variables) definition = replace_variables(definition, variables) return expand_for_each(definition, source_indices) @@ -331,26 +333,44 @@ def validation_errors(self, arguments=None): # tripped over - and reported where each sits rather than as a # for_each whose list arrived unsubstituted, which is what a # half-substituted definition used to look like from here - return self._undeclared_variable_errors() + return self._undeclared_variable_errors(arguments) return previous_result_reference_errors(expanded, source_indices) - def _undeclared_variable_errors(self): + def _undeclared_variable_errors(self, arguments=None): """Every 'variable:' reference naming nothing the workflow declares. Fatal rather than a warning: once a workflow has a 'variables' block, replace_variables refuses an undeclared reference, so this is - a run that cannot start. + a run that cannot start. Good caller `arguments` are folded in first, + and a reference inside one of them is reported under `arguments.`, + where the caller wrote it. """ - declared = sorted(self.workflow_definition.get("variables") or {}) + definition = copy.deepcopy(self.workflow_definition) + variables = definition.get("variables") + supplied = set() + if isinstance(variables, dict) and arguments: + if not argument_errors(definition, arguments): + set_variables(arguments, variables) + supplied = set(arguments) + declared = sorted(variables or {}) + + def where(path): + head, _, rest = path.partition(".") + if head == "variables": + name = rest.split(".", 1)[0].split("[", 1)[0] + if name in supplied: + return "arguments." + rest + return path + return [ { - "path": path, + "path": where(path), "message": ( f"'variable:{name}' names no declared variable; " f"declared: {', '.join(declared) or ''}" ), } - for path, name in undeclared_variable_references(self.workflow_definition) + for path, name in undeclared_variable_references(definition) ] def validate(self): @@ -438,6 +458,10 @@ def run( # first set variable values base don the arguments passed to the workflow # these may come form the command line or form a parent workflow set_variables(arguments, variables) + # an entry of a list-valued variable may name another + # variable; resolve those before anything inside it is + # realized, so a reference type in an entry is a type name + variables = resolve_variable_values(variables) # realize the variables, initialiting downloads of images etc realize_args(variables, base_dir) ## then replace any variable references in the workflow definition with the actual values diff --git a/tests/test_variables.py b/tests/test_variables.py index 4c84075b..2f2fe129 100644 --- a/tests/test_variables.py +++ b/tests/test_variables.py @@ -1,5 +1,12 @@ +import copy import pytest -from dw.variables import replace_variables, set_variables, VariableNotFoundError +from dw.variables import ( + replace_variables, + resolve_variable_values, + set_variables, + undeclared_variable_references, + VariableNotFoundError, +) def test_replace_variables_in_dict(): @@ -182,3 +189,83 @@ def test_set_variables_string_override_of_a_null_default_passes_through(): variables = {"mask": None} set_variables({"mask": "masks/a.png"}, variables) assert variables["mask"] == "masks/a.png" + + +class TestResolveVariableValues: + """A list-valued variable's entries may name other variables - a shot + entry says "from_file": "variable:character_a_voice" and one variable + sets the voice in every shot it speaks in.""" + + def test_a_reference_inside_a_list_value_is_replaced(self): + variables = { + "voice": "cast/priya.wav", + "shots": [{"name": "a", "references": [{"from_file": "variable:voice"}]}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"][0]["from_file"] == "cast/priya.wav" + + def test_a_reference_inside_a_dict_value_is_replaced(self): + variables = {"n": 124, "shape": {"num_frames": "variable:n"}} + assert resolve_variable_values(variables)["shape"] == {"num_frames": 124} + + def test_a_null_variable_resolves_to_null(self): + variables = { + "voice": None, + "shots": [{"references": [{"from_file": "variable:voice"}]}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"][0]["from_file"] is None + + def test_a_scalar_value_that_looks_like_a_reference_is_left_alone(self): + variables = {"x": "variable:y", "y": 1} + assert resolve_variable_values(variables)["x"] == "variable:y" + + def test_a_chain_resolves_through_a_referenced_list(self): + variables = { + "voice": "a.wav", + "refs": [{"from_file": "variable:voice"}], + "shots": [{"references": "variable:refs"}], + } + resolved = resolve_variable_values(variables) + assert resolved["shots"][0]["references"] == [{"from_file": "a.wav"}] + + def test_the_input_is_not_mutated(self): + variables = {"voice": "a.wav", "shots": [{"from_file": "variable:voice"}]} + before = copy.deepcopy(variables) + resolve_variable_values(variables) + assert variables == before + + def test_an_undeclared_name_is_the_usual_error(self): + with pytest.raises(VariableNotFoundError, match="nope"): + resolve_variable_values({"shots": [{"x": "variable:nope"}]}) + + def test_a_cycle_is_an_error_that_names_the_loop(self): + variables = {"a": [{"x": "variable:b"}], "b": [{"y": "variable:a"}]} + with pytest.raises(ValueError, match="a -> b -> a"): + resolve_variable_values(variables) + + def test_a_self_reference_is_a_cycle(self): + with pytest.raises(ValueError, match="a -> a"): + resolve_variable_values({"a": [{"x": "variable:a"}]}) + + +class TestUndeclaredReferencesInsideVariableValues: + def test_a_reference_inside_a_list_value_is_found_with_its_path(self): + definition = { + "variables": {"shots": [{"references": [{}, {"from_file": "variable:nope"}]}]}, + "steps": [], + } + assert undeclared_variable_references(definition) == [ + ("variables.shots[0].references[1].from_file", "nope") + ] + + def test_a_declared_reference_inside_a_value_is_not_reported(self): + definition = { + "variables": {"voice": None, "shots": [{"from_file": "variable:voice"}]}, + "steps": [], + } + assert undeclared_variable_references(definition) == [] + + def test_a_scalar_value_beginning_with_the_prefix_is_not_a_reference(self): + definition = {"variables": {"x": "variable:nope"}, "steps": []} + assert undeclared_variable_references(definition) == [] diff --git a/tests/test_workflow.py b/tests/test_workflow.py index cf2476fb..092d7d81 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -755,3 +755,56 @@ def test_run_substitutes_the_callers_list(tmp_path): workflow.run({"shots": [{"name": "only", "text": "X"}]}) names = [entry["step"] for entry in workflow.manifest] assert names == ["shot@only", "edit"] + + +def test_an_entry_may_reference_another_variable(tmp_path): + """A shot entry's "from_file": "variable:voice" is the voice variable's + value by the time the member exists.""" + definition = _for_each_workflow() + definition["variables"]["voice"] = "cast/priya.wav" + definition["variables"]["shots"] = [ + {"name": "a", "text": "one", "voice": "variable:voice"} + ] + definition["steps"][0]["task"]["arguments"]["voice"] = "item:voice" + workflow = _workflow_from(definition, tmp_path) + + expanded = workflow.expanded_definition() + + assert expanded["steps"][0]["task"]["arguments"]["voice"] == "cast/priya.wav" + + +def test_an_undeclared_reference_inside_a_default_entry_is_a_validation_error( + tmp_path, +): + definition = _for_each_workflow() + definition["variables"]["shots"] = [{"name": "a", "text": "variable:nope"}] + workflow = _workflow_from(definition, tmp_path) + + errors = workflow.validation_errors() + + assert [e["path"] for e in errors] == ["variables.shots[0].text"] + assert "names no declared variable" in errors[0]["message"] + + +def test_an_undeclared_reference_inside_a_caller_s_entry_is_reported_under_arguments( + tmp_path, +): + workflow = _workflow_from(_for_each_workflow(), tmp_path) + + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a", "text": "variable:nope"}]} + ) + + assert [e["path"] for e in errors] == ["arguments.shots[0].text"] + + +def test_a_caller_s_entry_may_reference_a_declared_variable(tmp_path): + definition = _for_each_workflow() + definition["variables"]["voice"] = None + workflow = _workflow_from(definition, tmp_path) + + errors = workflow.validation_errors( + arguments={"shots": [{"name": "a", "text": "variable:voice"}]} + ) + + assert errors == [] From 3ba86056285c7cc6ef8282794fe0d6c2ed772f2f Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:31:26 -0500 Subject: [PATCH 15/38] fix(catalog): a concat over gather: derives sequence Co-Authored-By: Claude Fable 5.1 --- dw/server/catalog_shape.py | 6 +++--- tests/test_catalog_shape.py | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/dw/server/catalog_shape.py b/dw/server/catalog_shape.py index b9a1cbd1..cc471dd8 100644 --- a/dw/server/catalog_shape.py +++ b/dw/server/catalog_shape.py @@ -160,13 +160,13 @@ def _needs_input_media(steps): def _cuts_together(steps): """A concat or dissolve fed by two or more distinct steps, or by a list of shots handed in whole - one `variable:` reference is a supplied list - whose length only the caller knows, and a cut over supplied footage is - still an edit.""" + whose length only the caller knows, one `gather:` reference is every + member of a for_each group, and a cut over either is still an edit.""" for step in steps: key, body = _block(step) if key == "task" and body.get("command") in _CUT_TASKS: videos = _arguments(step).get("videos") - if isinstance(videos, str) and videos.startswith("variable:"): + if isinstance(videos, str) and videos.startswith(("variable:", "gather:")): return True sources = _fed_by(videos) if isinstance(videos, list): diff --git a/tests/test_catalog_shape.py b/tests/test_catalog_shape.py index 14a75465..eb0cccbf 100644 --- a/tests/test_catalog_shape.py +++ b/tests/test_catalog_shape.py @@ -159,6 +159,25 @@ def test_a_concat_fed_by_two_steps_is_a_sequence(): assert meta["shape"] == "sequence" +def test_a_concat_over_a_gathered_for_each_group_is_a_sequence(): + """A list-driven template's editor says "videos": "gather:shot" - a + cut over as many shots as the list holds, which the file cannot count.""" + meta = derive_catalog_metadata( + definition( + { + "name": "shot", + "for_each": "variable:shots", + "pipeline": { + "arguments": {"prompt": "item:prompt"}, + }, + "result": {"content_type": "video/mp4"}, + }, + task_step("cut", "concat_videos", {"videos": "gather:shot"}, "video/mp4"), + ) + ) + assert meta["shape"] == "sequence" + + def test_a_dissolve_fed_by_two_steps_is_a_sequence(): meta = derive_catalog_metadata( definition( From 36a7383f260e49bc7b52a52c7a367db017c54691 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:33:47 -0500 Subject: [PATCH 16/38] fix(variables): review round 1 - black formatting, cycle as validation error, fold lookup A variable cycle now surfaces as a validation error at path "variables" instead of escaping as a bare ValueError. resolve_variable_values reuses _resolve_variable_reference for the "variable:name" lookup instead of duplicating its not-found message. Co-Authored-By: Claude Fable 5.1 --- dw/variables.py | 11 +++-------- dw/workflow.py | 7 +++++++ tests/test_variables.py | 4 +++- tests/test_workflow.py | 14 ++++++++++++++ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/dw/variables.py b/dw/variables.py index f774c7ea..ffbda719 100644 --- a/dw/variables.py +++ b/dw/variables.py @@ -121,14 +121,9 @@ def resolve(name, chain): return value def walk(node, chain): - if isinstance(node, str) and node.startswith("variable:"): - target = node.removeprefix("variable:") - if target not in variables: - available = ", ".join(sorted(variables.keys())) or "" - raise VariableNotFoundError( - f"Variable <{target}> not found; available variables: {available}" - ) - return resolve(target, chain) + matched, _ = _resolve_variable_reference(node, variables) + if matched: + return resolve(node.removeprefix("variable:"), chain) if isinstance(node, list): return [walk(item, chain) for item in node] if isinstance(node, dict): diff --git a/dw/workflow.py b/dw/workflow.py index 44a34457..06961786 100644 --- a/dw/workflow.py +++ b/dw/workflow.py @@ -334,6 +334,13 @@ def validation_errors(self, arguments=None): # for_each whose list arrived unsubstituted, which is what a # half-substituted definition used to look like from here return self._undeclared_variable_errors(arguments) + except ValueError as e: + # resolve_variable_values raises a bare ValueError for a variable + # that references itself, directly or through others - there is + # no single path inside the definition to blame, so it is + # reported against 'variables' as a whole rather than escaping + # as an unhandled exception + return [{"path": "variables", "message": str(e)}] return previous_result_reference_errors(expanded, source_indices) def _undeclared_variable_errors(self, arguments=None): diff --git a/tests/test_variables.py b/tests/test_variables.py index 2f2fe129..188f4ca6 100644 --- a/tests/test_variables.py +++ b/tests/test_variables.py @@ -252,7 +252,9 @@ def test_a_self_reference_is_a_cycle(self): class TestUndeclaredReferencesInsideVariableValues: def test_a_reference_inside_a_list_value_is_found_with_its_path(self): definition = { - "variables": {"shots": [{"references": [{}, {"from_file": "variable:nope"}]}]}, + "variables": { + "shots": [{"references": [{}, {"from_file": "variable:nope"}]}] + }, "steps": [], } assert undeclared_variable_references(definition) == [ diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 092d7d81..0c06e453 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -808,3 +808,17 @@ def test_a_caller_s_entry_may_reference_a_declared_variable(tmp_path): ) assert errors == [] + + +def test_a_variable_cycle_is_a_validation_error_at_variables(tmp_path): + definition = _for_each_workflow() + definition["variables"] = { + "a": [{"x": "variable:b"}], + "b": [{"y": "variable:a"}], + } + workflow = _workflow_from(definition, tmp_path) + + errors = workflow.validation_errors() + + assert [e["path"] for e in errors] == ["variables"] + assert "a -> b -> a" in errors[0]["message"] From 5d8bfcdd5fa43686ba28ef68df1f63b3420a0c75 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Fri, 11 Sep 2026 19:40:23 -0500 Subject: [PATCH 17/38] feat(templates): music-video's slices and shots are two for_each groups over one shots list Breaking for scripted callers: shot_1_wide_open .. shot_4_finale are entries of 'shots' now, each {name, prompt, start_frame}. Co-Authored-By: Claude Fable 5.1 --- tests/test_for_each.py | 138 +++++++------- workflows/templates/minimax/README.md | 2 +- workflows/templates/minimax/music-video.json | 185 ++++--------------- 3 files changed, 98 insertions(+), 227 deletions(-) diff --git a/tests/test_for_each.py b/tests/test_for_each.py index b26d1d96..eceb9966 100644 --- a/tests/test_for_each.py +++ b/tests/test_for_each.py @@ -26,6 +26,12 @@ def load_template(name): return json.load(f) +def load_workflow(name): + from dw.workflow import workflow_from_file + + return workflow_from_file(os.path.normpath(os.path.join(TEMPLATES, name)), ".") + + def steps_by_name(definition): return {s["name"]: s for s in definition["steps"]} @@ -628,86 +634,74 @@ def test_a_gather_inside_a_member_still_gathers(self): class TestMusicVideoTemplate: - """music-video's four slices and four shots, written as two for_each - groups over one 'shots' list, expand to the steps the template holds - by hand today.""" + """music-video's slices and shots are two for_each groups over one + 'shots' list, paired by entry name: shot@closeup reads slice@closeup.""" - def test_the_hand_written_shots_are_what_the_list_expands_to(self): - template = load_template("music-video.json") - today = steps_by_name(template) - shots = [ - { - "name": "wide_open", - "prompt": "variable:shot_1_wide_open", - "start_frame": 0, - }, - { - "name": "closeup", - "prompt": "variable:shot_2_closeup", - "start_frame": 124, - }, - {"name": "room", "prompt": "variable:shot_3_room", "start_frame": 248}, - {"name": "finale", "prompt": "variable:shot_4_finale", "start_frame": 372}, - ] - slice_template = copy.deepcopy(today["slice_1"]) - slice_template["name"] = "slice" - slice_template["for_each"] = shots - slice_template["task"]["arguments"]["start_frame"] = "item:start_frame" + KEYS = ["wide_open", "closeup", "room", "finale"] - shot_template = copy.deepcopy(today["shot_1_wide_open"]) - shot_template["name"] = "shot" - shot_template["for_each"] = shots - shot_template["pipeline"]["arguments"]["prompt"] = "item:prompt" - for reference in shot_template["pipeline"]["arguments"]["references"]: - if reference.get("from_previous_result") == "slice_1": - reference["from_previous_result"] = "slice" + def expanded(self): + return load_workflow("music-video.json").expanded_definition() - edit = copy.deepcopy(today["edit"]) - edit["task"]["arguments"]["videos"] = "gather:shot" + def test_the_template_validates_as_it_will_run(self): + assert load_workflow("music-video.json").validation_errors() == [] - expanded = expand_for_each( - definition( - today["draw_singer"], - today["write_song"], - slice_template, - today["soundtrack"], - shot_template, - edit, - today["music_video"], - ) + def test_one_slice_and_one_shot_per_entry_in_list_order(self): + names = [s["name"] for s in self.expanded()["steps"]] + assert names == ( + ["draw_singer", "write_song"] + + [f"slice@{k}" for k in self.KEYS] + + ["soundtrack"] + + [f"shot@{k}" for k in self.KEYS] + + ["edit", "music_video"] ) - got = steps_by_name(expanded) - # Each expanded slice is today's slice with the new name - for key, old in zip(["wide_open", "closeup", "room", "finale"], range(1, 5)): - expected = copy.deepcopy(today[f"slice_{old}"]) - expected["name"] = f"slice@{key}" - assert got[f"slice@{key}"] == expected - - # Each expanded shot is today's shot (as a full pipeline block) with - # the new name and its slice renamed - hand_written = [ - "shot_1_wide_open", - "shot_2_closeup", - "shot_3_room", - "shot_4_finale", + def test_each_slice_starts_where_its_entry_says(self): + got = steps_by_name(self.expanded()) + starts = [ + got[f"slice@{k}"]["task"]["arguments"]["start_frame"] for k in self.KEYS ] - for key, old, index in zip( - ["wide_open", "closeup", "room", "finale"], hand_written, range(1, 5) - ): - step = today[old] - if "pipeline_reference" in step: - step = without_pipeline_reference(step, today["shot_1_wide_open"]) - expected = copy.deepcopy(step) - expected["name"] = f"shot@{key}" - for reference in expected["pipeline"]["arguments"]["references"]: - if reference.get("from_previous_result") == f"slice_{index}": - reference["from_previous_result"] = f"slice@{key}" - assert got[f"shot@{key}"] == expected - + assert starts == [0, 124, 248, 372] + + def test_each_shot_reads_its_own_slice_and_the_one_portrait(self): + got = steps_by_name(self.expanded()) + for key in self.KEYS: + references = got[f"shot@{key}"]["pipeline"]["arguments"]["references"] + assert [r["from_previous_result"] for r in references] == [ + "draw_singer", + f"slice@{key}", + ] + + def test_each_shot_carries_its_entry_s_prompt(self): + template = load_template("music-video.json") + got = steps_by_name(self.expanded()) + for entry in template["variables"]["shots"]: + prompt = got[f"shot@{entry['name']}"]["pipeline"]["arguments"]["prompt"] + assert prompt == entry["prompt"] + assert prompt.startswith("subject_definitions:") + + def test_every_shot_is_the_same_pipeline(self): + """Full pipeline blocks rather than pipeline_reference: the identity + cache reuses the loaded model, so this costs no reload.""" + from dw.workflow import pipeline_cache_key + + got = steps_by_name(self.expanded()) + keys = {pipeline_cache_key(got[f"shot@{k}"]["pipeline"]) for k in self.KEYS} + assert len(keys) == 1 + + def test_the_edit_gathers_the_shots_in_order(self): + got = steps_by_name(self.expanded()) assert got["edit"]["task"]["arguments"]["videos"] == [ - f"previous_result:shot@{k}" - for k in ["wide_open", "closeup", "room", "finale"] + f"previous_result:shot@{k}" for k in self.KEYS + ] + + def test_the_realized_file_keeps_the_list(self): + """workflow.json beside a run is the source form: 'for_each' and the + 'shots' variable, not the expanded members.""" + template = load_template("music-video.json") + assert "shots" in template["variables"] + assert [s["name"] for s in template["steps"] if "for_each" in s] == [ + "slice", + "shot", ] diff --git a/workflows/templates/minimax/README.md b/workflows/templates/minimax/README.md index 7ff7e899..29cdd5ea 100644 --- a/workflows/templates/minimax/README.md +++ b/workflows/templates/minimax/README.md @@ -133,4 +133,4 @@ same clip as an audio reference in each shot, as | Example | What it introduces | | ------- | ------------------ | | [dialogue-short.json](dialogue-short.json) | A five-shot sitcom scene: Z-Image draws the cast, `pipeline_reference` reruns one loaded model per shot, `concat_videos` splices the episode | -| [music-video.json](music-video.json) | A music video cut to a generated song: `slice_audio` deals frame-exact pieces to lip-synced shots, and `pair_audio` lays the unbroken track over the finished edit | +| [music-video.json](music-video.json) | A music video cut to a generated song, one `shots` list driving both `for_each` groups: `slice_audio` deals each entry its frame-exact piece, the shot lip-syncs to it, and `pair_audio` lays the unbroken track over the finished edit | diff --git a/workflows/templates/minimax/music-video.json b/workflows/templates/minimax/music-video.json index 0e0b4747..db77fad6 100644 --- a/workflows/templates/minimax/music-video.json +++ b/workflows/templates/minimax/music-video.json @@ -1,6 +1,6 @@ { "id": "MiniMaxH3MusicVideo", - "description": "A music video built from cuts, sung to a soundtrack that never touches a chain. The long-take way to film a song - one chained generation lip-synced end to end - degrades with every carried segment and can let the sync slip. This builds the video the way music television does instead: MiniMax-Music3 writes the song, 'slice_audio' cuts it into frame-exact pieces (124 frames at 24 fps each), and every shot is generated fresh from the same Z-Image portrait plus its own slice, lip-synced to just those five seconds. No shot conditions on another shot's output, so the last cut is as clean as the first. 'concat_videos' splices the shots, and because each one covered exactly its slice's frames, the edit is sample-accurate by construction: 'pair_audio' drops the original, unbroken song over the whole cut and the mouths line up in every shot. The generation models pass through one at a time - each is released before the next loads - so the workflow peaks no higher than its largest single model.", + "description": "A music video built from cuts, sung to a soundtrack that never touches a chain. The long-take way to film a song - one chained generation lip-synced end to end - degrades with every carried segment and can let the sync slip. This builds the video the way music television does instead: MiniMax-Music3 writes the song, one 'slice_audio' step per entry of 'shots' cuts it into frame-exact pieces (124 frames at 24 fps each, from each entry's 'start_frame'), and one shot per entry is generated fresh from the same Z-Image portrait plus its own slice, lip-synced to just those five seconds. 'shots' is a list, so the cut is an argument: add an entry and there is one more slice and one more shot, named for it ('slice@closeup', 'shot@closeup'), and the loaded model is reused across every shot - identical pipeline definitions share one model. No shot conditions on another shot's output, so the last cut is as clean as the first. 'concat_videos' gathers the shots in list order, and because each one covered exactly its slice's frames, the edit is sample-accurate by construction: 'pair_audio' drops the original, unbroken song over the whole cut and the mouths line up in every shot. The generation models pass through one at a time - each is released before the next loads - so the workflow peaks no higher than its largest single model.", "cost": [ {"device": "cuda", "name": "RTX 3090", "vram_gb": 24, "minutes": 35} ], @@ -9,10 +9,28 @@ "song_prompt": "prompt:minimax/otter_soul_song", "song_lyrics": "[verse]\nI've been waiting on you darling like a render overnight\nEvery frame of my devotion coming slowly into light\n[chorus]\nOoh, progress bar of love\nOoh, don't you stall on me now", "audio_duration": 30, - "shot_1_wide_open": "subject_definitions:\n is the anthropomorphic river otter lounge singer in , in a burgundy velvet dinner jacket and black bow tie, whose face, fur, jacket and proportions define his appearance for the whole video.\n