From cd8505a05638218764d1af52593aa141aa1dffec Mon Sep 17 00:00:00 2001 From: Jeremy Bedient Date: Thu, 13 Aug 2026 14:17:44 -0400 Subject: [PATCH 1/2] Add automations diff to preview a spec against the live automation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `automations update` was all-or-nothing: you either trusted the spec or eyeballed two full JSON trees. `diff` shows what the update would change — trigger/step additions, removals, reparenting, and field changes. The hard part is identity. GET and PUT are different dialects, so `key` is resynthesized from live order on one side and hand-authored on the other; comparing them literally reports every step as changed. Steps and triggers are matched by `id`, with `key`/`parent_key`/`prefix` excluded as per-side naming rather than content. Reparenting and `go_to_automation_step` references are resolved to the matched identity of their target, so they still surface a real change without tripping on a cosmetic rekey. A spec item carrying an `id` that matches no live step is treated as an addition, and the live step it would have displaced as a removal, rather than merged into it by position — that is what the PUT would actually do, and the previous position-merge silently swallowed the unknown id. --- CHANGELOG.md | 15 ++ scripts/cli-tree-baseline.txt | 33 +++ src/kizen_builder/cli/automations.py | 74 ++++++- src/kizen_builder/docs/commands.md | 1 + src/kizen_builder/docs/specs/automation.md | 23 ++ .../tools/planners/automations.py | 59 +++++ src/kizen_builder/translate.py | 209 ++++++++++++++++++ tests/test_automation_payloads.py | 132 +++++++++++ tests/test_cli.py | 84 +++++++ tests/test_translate.py | 204 +++++++++++++++++ 10 files changed, 832 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bea17e..c98bf56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,6 +141,21 @@ called out explicitly under **Changed** or **Removed**. `tests/fixtures/examples/service_ticket/`, checked two ways: an offline test that fails if the doc and the fixtures ever diverge, and an opt-in drift test that applies the same fixtures live. +- **`kizen automations diff --spec-file `** (stdin also + accepted) previews what `automations update` from that spec would actually + change on the live automation — trigger/step additions, removals, + reparenting, and config-field changes — without writing anything. Steps and + triggers are matched by `id` first, position as a fallback for a spec with + no `id`s at all; `key`/`parent_key`/`prefix` are excluded from the + comparison since they're per-side synthetic naming, not automation content, + so an unchanged spec produces an empty diff instead of showing every step's + resynthesized `key` as "changed." Each diff line is labelled with the first + octet of the step/trigger's `id` (matching what's visible in the UI), which + is unique within a single automation; under `--json`, an addition or removal + also carries the whole step/trigger including its full `id`. + `kizen automations get`'s Steps table also + gains an `id` column (first octet) and shortens `parent` to match, so the + two can be read against each other without `--json`. - **The package declares its license.** `kizen-builder` is MIT-licensed, and the built wheel and sdist now carry `License-Expression: MIT` along with a copy of `LICENSE`. diff --git a/scripts/cli-tree-baseline.txt b/scripts/cli-tree-baseline.txt index ba4bec8..810db06 100644 --- a/scripts/cli-tree-baseline.txt +++ b/scripts/cli-tree-baseline.txt @@ -437,6 +437,8 @@ │ business_plugin_app_id (the value `call_llm.business_plugin_app_id` needs │ │ for any non-`kizen/*` model_name — see automation.md). │ │ roundtrip Verify GET→PUT translation fidelity for one automation. │ +│ diff Show what `automations update` from this spec would change on the │ +│ live automation — read-only, no write is made. │ │ show Render the automation as a step tree with stable step keys. │ │ start Trigger an automation, optionally on a record and seeding variables. │ │ modification-history Who changed this automation, when, and what changed. │ @@ -520,6 +522,34 @@ │ --help Show this message and exit. │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ + Usage: automations diff [OPTIONS] {api_name} + + Show what `automations update` from this spec would change on the live automation — read-only, no + write is made. + + Triggers/steps are matched by `id` first (regardless of `key`/order), + position among the rest as a fallback for a spec with no `id`s at all. + `key`/`parent_key`/`prefix` are excluded from the comparison — they're + per-side synthetic naming (see `kizen docs show automation`), not + automation content — but a genuine reparenting still shows, compared by + matched identity rather than raw key. Each line is labelled with the + first octet of the step/trigger's `id` so it can be matched to what's + visible in the UI; it is unique within one automation. Under `--json`, an + added or removed step/trigger also carries its full `id` in the leaf + value. + +╭─ Arguments ──────────────────────────────────────────────────────────────────────────────────────╮ +│ * api_name Automation api_name. [required] │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Options ────────────────────────────────────────────────────────────────────────────────────────╮ +│ --spec-file Path to JSON AutomationDef. Default: read from stdin. │ +│ --json Emit full result as JSON. │ +│ --help Show this message and exit. │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ + + Spec shape (an AutomationDef: triggers + step graph): see `kizen docs show automation` + + Usage: automations duplicate [OPTIONS] {api_name} Duplicate one automation. The copy is named/numbered by the server. @@ -4069,6 +4099,9 @@ ===== automations delete ===== +===== automations diff ===== + + ===== automations duplicate ===== diff --git a/src/kizen_builder/cli/automations.py b/src/kizen_builder/cli/automations.py index a7bab4e..24fd6de 100644 --- a/src/kizen_builder/cli/automations.py +++ b/src/kizen_builder/cli/automations.py @@ -9,19 +9,23 @@ from typing import Any import typer +from pydantic import ValidationError from rich.table import Table from kizen_builder import output as out from kizen_builder.api.client import KizenAPIError +from kizen_builder.cli._mutations import _read_spec from kizen_builder.cli._shared import ( JSON_OPTION, OUTPUT_OPTION, app, cli_errors, console, + err_console, ) from kizen_builder.tools import automations as auto_tools from kizen_builder.tools import steps as step_tools +from kizen_builder.tools.planners import automations as auto_planners from kizen_builder.tools.plans import PlanError autos_app = typer.Typer( @@ -122,16 +126,18 @@ def table() -> None: step_table = Table(title="Steps") step_table.add_column("ord", justify="right") + step_table.add_column("id", style="dim") step_table.add_column("type") step_table.add_column("description") - step_table.add_column("parent") + step_table.add_column("parent", style="dim") step_table.add_column("branch") for s in result["steps"]: step_table.add_row( str(s["order"]) if s["order"] is not None else "", + (s["id"] or "")[:8], s["step_type"] or "", s["description"] or "", - (s["parent_step_id"] or ""), + (s["parent_step_id"] or "")[:8], s["parent_condition"] or "", ) console.print(step_table) @@ -274,6 +280,70 @@ def autos_roundtrip( raise typer.Exit(code=1) +@autos_app.command( + "diff", + epilog="Spec shape (an AutomationDef: triggers + step graph): see `kizen docs show automation`", +) +def autos_diff( + api_name: str = typer.Argument(..., help="Automation api_name."), + spec_file: str = typer.Option( + "", + "--spec-file", + help="Path to JSON AutomationDef. Default: read from stdin.", + ), + json_out: bool = typer.Option(False, "--json", help="Emit full result as JSON."), +) -> None: + """Show what `automations update` from this spec would change on the + live automation — read-only, no write is made. + + Triggers/steps are matched by `id` first (regardless of `key`/order), + position among the rest as a fallback for a spec with no `id`s at all. + `key`/`parent_key`/`prefix` are excluded from the comparison — they're + per-side synthetic naming (see `kizen docs show automation`), not + automation content — but a genuine reparenting still shows, compared by + matched identity rather than raw key. Each line is labelled with the + first octet of the step/trigger's `id` so it can be matched to what's + visible in the UI; it is unique within one automation. Under `--json`, an + added or removed step/trigger also carries its full `id` in the leaf + value. + """ + spec_dict, _from_stdin = _read_spec(spec_file) + spec_api_name = spec_dict.get("api_name") + if spec_api_name and spec_api_name != api_name: + err_console.print( + f"[red]error:[/red] spec api_name '{spec_api_name}' does not match " + f"'{api_name}' — diffing against the wrong automation. Pass the " + "spec's own api_name, or fix --spec-file." + ) + raise typer.Exit(code=2) + try: + with cli_errors(LookupError, PlanError): + result = auto_planners.diff_automation(spec_dict) + except ValidationError as e: + for err in e.errors(): + loc = ".".join(str(p) for p in err.get("loc", ())) or "spec" + msg = err.get("msg", "invalid value") + err_console.print(f"[red]spec error:[/red] {loc}: {msg}") + raise typer.Exit(code=1) from e + + if json_out: + typer.echo(json.dumps(result, indent=2)) + return + + console.print( + f"[bold]{result['api_name']}[/bold] [dim](rev {result['revision']})[/dim]" + ) + diff = result["diff"] + if not diff: + console.print("[green]no changes[/green] — update would be a no-op") + return + console.print(f"[yellow]{len(diff)} change(s):[/yellow]") + for d in diff: + console.print( + f" [yellow]{d['path']}[/yellow]: {d['before']!r} → {d['after']!r}" + ) + + def _step_label(step: dict[str, Any]) -> str: desc = (step.get("description") or step.get("user_description") or "").strip() if len(desc) > 72: diff --git a/src/kizen_builder/docs/commands.md b/src/kizen_builder/docs/commands.md index f9749bd..7738e4b 100644 --- a/src/kizen_builder/docs/commands.md +++ b/src/kizen_builder/docs/commands.md @@ -21,6 +21,7 @@ kizen automations get # one automation incl. triggers + s kizen automations show # step tree with synthesized step keys (handles for steps verbs) kizen automations steps get # one step's wire JSON (starting point for steps edit) kizen automations roundtrip # translate + graph-validate (add --execute to PUT + drift-check) +kizen automations diff --spec-file # preview what `update` from this spec would change — read-only kizen automations llm-models # live model_name + business_plugin_app_id catalog (see kizen docs show automation) kizen automations runs list # recent runs for an automation kizen automations runs view # one run: summary + step-by-step trace (per-step status/duration) diff --git a/src/kizen_builder/docs/specs/automation.md b/src/kizen_builder/docs/specs/automation.md index a66c1c4..bbe945a 100644 --- a/src/kizen_builder/docs/specs/automation.md +++ b/src/kizen_builder/docs/specs/automation.md @@ -566,6 +566,29 @@ Why a translator is mandatory rather than nice-to-have: Also: **automation updates need PUT, not PATCH.** PATCH refuses step/trigger changes, and PUT requires the current `revision` as `last_revision`. +### Previewing an update before you send it + +`kizen automations diff --spec-file ` (stdin also accepted) +shows what `automations update` from that spec would actually change on the +live automation — trigger/step additions, removals, reparenting, and +config-field changes — without writing anything. It normalizes both sides +into the wire (PUT) dialect above and matches steps/triggers by `id` first, +falling back to position for a spec with no `id`s at all (see "A +step/trigger's `id` field controls..." above for what setting `id` does). + +Because `key` is synthesized fresh from live order on one side and +hand-authored on the other, a literal field comparison would show every +step's `key`/`parent_key` as "changed" even when nothing actually changed. +`diff` excludes `key`, `parent_key`, and `prefix` from the comparison — they +are per-side synthetic naming, not automation content — but still catches a +genuine reparenting by comparing each step's parent by matched identity, not +by the raw key string. Each diff line is labelled with the first octet of +the step/trigger's `id` (e.g. `76af48bd`) so it can be matched against the +same value visible in the UI, and it is unique within one automation. A +changed field is identified by that octet alone; an added or removed +step/trigger carries the whole step/trigger, full `id` included, in its +`--json` leaf value. + ### Read→write transforms Each of these was discovered via a live 400, 500, or silent data loss: diff --git a/src/kizen_builder/tools/planners/automations.py b/src/kizen_builder/tools/planners/automations.py index a9b6dda..e92f3b4 100644 --- a/src/kizen_builder/tools/planners/automations.py +++ b/src/kizen_builder/tools/planners/automations.py @@ -296,6 +296,65 @@ def plan_update_automation(automation: dict[str, Any] | AutomationDef) -> Plan: ) +def diff_automation(automation: dict[str, Any] | AutomationDef) -> dict[str, Any]: + """Compare a spec's would-be `update` against the live automation, + without writing anything — the read-only counterpart to + `plan_update_automation`. + + Fetches the live automation exactly once and derives both comparison + sides from that single `current`: the live side via + `translate.live_to_payload`, the spec side via the same + `_build_automation_payload` + `_merge_server_state` calls + `plan_update_automation` itself makes (not by calling + `plan_update_automation`/building a `Plan` a second time, which would + re-fetch `current` and admit a race). This also guarantees + `last_revision` matches on both sides by construction. Mirrors + `plan_update_automation`'s `active` resolution (an omitted `active` in + the spec means "leave live's value alone," not "set False") so `active` + only shows a diff when the spec genuinely asks for a flip. + + See `translate.diff_wire_payloads` for the id-first/position-fallback + step and trigger matching, and the `key`/`parent_key`/`prefix` + exclusions that keep synthesized-key churn out of the result. + """ + from kizen_builder.translate import diff_wire_payloads, live_to_payload + + auto = ( + automation + if isinstance(automation, AutomationDef) + else AutomationDef.model_validate(automation) + ) + ctx = LiveContext() + env = ctx.env + + existing = next( + (a for a in list_automations() if a["api_name"] == auto.api_name), + None, + ) + if existing is None: + raise PlanError( + f"no automation with api_name '{auto.api_name}'. " + "Use plan_create_automation." + ) + + current = get_automation(auto.api_name)["raw"] + + live_active = bool(current.get("active", False)) + resolved_active = auto.active if auto.active is not None else live_active + auto = auto.model_copy(update={"active": resolved_active}) + + live_payload = live_to_payload(current) + spec_payload = _merge_server_state(_build_automation_payload(auto, ctx), current) + + return { + "env": env, + "api_name": auto.api_name, + "id": existing["id"], + "revision": current.get("revision"), + "diff": diff_wire_payloads(live_payload, spec_payload), + } + + def _merge_server_state( payload: dict[str, Any], current: dict[str, Any] ) -> dict[str, Any]: diff --git a/src/kizen_builder/translate.py b/src/kizen_builder/translate.py index 3e03694..14f88f5 100644 --- a/src/kizen_builder/translate.py +++ b/src/kizen_builder/translate.py @@ -483,3 +483,212 @@ def _diff(a: Any, b: Any, path: str) -> list[tuple[str, Any, Any]]: if a != b: return [(path, a, b)] return [] + + +# --------------------------------------------------------------------------- +# Wire diff (live payload vs. spec-as-applied payload) +# --------------------------------------------------------------------------- + +# Per-side synthetic naming, not automation content — excluded from +# comparison so a spec that changes nothing produces an empty diff even +# though `key` is resynthesized from live order on one side and authored by +# hand on the other (see `automation.md`'s "GET and PUT are different +# dialects"). +_WIRE_DIFF_EXCLUDED = {"key", "parent_key", "prefix"} + + +def _wire_pairs( + live_items: list[dict[str, Any]], spec_items: list[dict[str, Any]] +) -> list[tuple[dict[str, Any] | None, dict[str, Any] | None]]: + """Match live and spec steps/triggers: by ``id`` when both sides have + one, then by position among what's left. + + Position fallback covers a spec authored from scratch with no ``id`` at + all — the same trick :func:`canonicalize` uses for round-trip identity, + adapted to match across two different payloads instead of one payload + before/after a write. A spec item that *does* carry an ``id``, but one + that matches no live item, is not a position-fallback candidate: a real + ``id`` naming a step that doesn't exist live cannot be that step under + any interpretation, so it is always an addition (and whatever live item + is left over is a removal), never merged into an unrelated step by + position. + """ + spec_by_id = {s["id"]: s for s in spec_items if s.get("id")} + consumed: set[int] = set() + pairs: list[tuple[dict[str, Any] | None, dict[str, Any] | None]] = [] + leftover_live: list[dict[str, Any]] = [] + for live_item in live_items: + match = spec_by_id.get(live_item.get("id")) + if match is not None and id(match) not in consumed: + pairs.append((live_item, match)) + consumed.add(id(match)) + else: + leftover_live.append(live_item) + + leftover_spec = [s for s in spec_items if id(s) not in consumed] + dangling_spec = [s for s in leftover_spec if s.get("id")] + unidentified_spec = [s for s in leftover_spec if not s.get("id")] + + pairs.extend((None, s) for s in dangling_spec) + for i in range(max(len(leftover_live), len(unidentified_spec))): + pairs.append( + ( + leftover_live[i] if i < len(leftover_live) else None, + unidentified_spec[i] if i < len(unidentified_spec) else None, + ) + ) + return pairs + + +def _wire_pair_id( + pair: tuple[dict[str, Any] | None, dict[str, Any] | None], +) -> str | None: + """The real id this matched pair shares — live's if it has one (it + always does; ``live_to_payload`` echoes every step/trigger's id), + otherwise spec's, otherwise ``None`` for a brand-new step/trigger the + spec hasn't assigned one to yet.""" + live_item, spec_item = pair + if live_item and live_item.get("id"): + return live_item["id"] + if spec_item and spec_item.get("id"): + return spec_item["id"] + return None + + +def _wire_pair_label( + pair: tuple[dict[str, Any] | None, dict[str, Any] | None], +) -> str: + """The path label for one matched pair: the first octet of its id — the + same ``[:8]`` convention as `tools/planners/dashboards.py`'s plan preview + and `tools/plans.py`'s plan id — so a diff line can be correlated with + what's visible in the UI. Falls back to the spec-authored `key` only for + a brand-new step/trigger with no id anywhere yet. + """ + pair_id = _wire_pair_id(pair) + if pair_id: + return pair_id[:8] + live_item, spec_item = pair + key = (spec_item or live_item or {}).get("key") or "?" + return f"new:{key}" + + +def _wire_key_labels( + pairs: list[tuple[dict[str, Any] | None, dict[str, Any] | None]], side: int +) -> dict[str, str]: + """Map one side's raw ``key`` to its pair's label, for resolving + ``parent_key`` into matched identity (see `_normalize_wire_steps`).""" + labels: dict[str, str] = {} + for pair in pairs: + item = pair[side] + if item is not None and item.get("key") is not None: + labels[item["key"]] = _wire_pair_label(pair) + return labels + + +def _normalize_wire_triggers( + pairs: list[tuple[dict[str, Any] | None, dict[str, Any] | None]], side: int +) -> dict[str, Any]: + out: dict[str, Any] = {} + for pair in pairs: + item = pair[side] + if item is None: + continue + norm = {k: v for k, v in item.items() if k not in _WIRE_DIFF_EXCLUDED | {"id"}} + # `id` is carried as a normal field, but pinned to the pair's shared + # canonical value on both sides so a matched pair never diffs on it + # (position-fallback matches can have an id on only one side) while + # still surfacing the real, untruncated id for `--json` on a bare + # addition/removal. + norm["id"] = _wire_pair_id(pair) + out[_wire_pair_label(pair)] = norm + return out + + +_GO_TO_BLOCK_FIELD = _block_field_for("go_to_automation_step") + + +def _normalize_wire_steps( + step_pairs: list[tuple[dict[str, Any] | None, dict[str, Any] | None]], + trigger_pairs: list[tuple[dict[str, Any] | None, dict[str, Any] | None]], + side: int, +) -> dict[str, Any]: + step_key_labels = _wire_key_labels(step_pairs, side) + trigger_key_labels = _wire_key_labels(trigger_pairs, side) + out: dict[str, Any] = {} + for pair in step_pairs: + item = pair[side] + if item is None: + continue + norm = {k: v for k, v in item.items() if k not in _WIRE_DIFF_EXCLUDED | {"id"}} + norm["id"] = _wire_pair_id(pair) + # Reparenting must still be visible: resolve `parent_key` to the + # *matched identity* of the parent (mirroring `canonicalize`'s + # `` markers) instead of dropping it outright — a spec that + # genuinely moves a step under a different parent shows a real + # `parent` change even though raw `parent_key` strings are excluded. + parent_key = item.get("parent_key") + norm["parent"] = step_key_labels.get(parent_key) if parent_key else None + # A `go_to_automation_step` reference is the same situation as + # `parent_key`: it names its target by this side's own `key`, which + # is separately synthesized (live) or authored (spec). Resolve it to + # the target's matched-pair identity so pointing at the *same* step + # compares equal across a cosmetic rekey; an unresolvable key (no + # matching pair) is left as-is, so a genuine retarget or a dangling + # reference still surfaces as a real change. + go_to = norm.get(_GO_TO_BLOCK_FIELD) + if isinstance(go_to, dict): + resolved = dict(go_to) + if resolved.get("step_key") is not None: + resolved["step_key"] = step_key_labels.get( + resolved["step_key"], resolved["step_key"] + ) + if resolved.get("trigger_key") is not None: + resolved["trigger_key"] = trigger_key_labels.get( + resolved["trigger_key"], resolved["trigger_key"] + ) + norm[_GO_TO_BLOCK_FIELD] = resolved + out[_wire_pair_label(pair)] = norm + return out + + +def diff_wire_payloads( + live: dict[str, Any], spec: dict[str, Any] +) -> list[dict[str, Any]]: + """Compare two PUT-dialect payloads for the *same* automation — the live + automation (via :func:`live_to_payload`) against the payload a spec's + `automations update` would send — and return what would actually change. + + Unlike :func:`semantic_diff` (GET-dialect, same automation before/after a + write, steps identified by position because the structure can't change), + the two sides here can legitimately differ in structure: a step can be + added, removed, or reparented. Steps/triggers are matched by `id` first + (regardless of `key`/order); position among the remainder is a fallback + only for spec items with no `id` at all — see :func:`_wire_pairs`. + `key`/`parent_key`/`prefix` are excluded from the field-by-field + comparison as per-side synthetic naming, not automation content; a + `go_to_automation_step` reference is resolved to its target's matched + identity the same way `parent_key` is, so it survives key resynthesis too. + + Returns ``[{"path", "before", "after"}, ...]`` — the same shape + `roundtrip_automation`'s `drift` field already uses. A step/trigger only + on one side (`before` or `after` is the literal `""` sentinel) is + an addition or removal; anything else is a changed field. `path` embeds + each step/trigger's first-id-octet label so a line can be matched to the + UI; the full id still travels in the leaf `before`/`after` values for + additions/removals (an addition/removal's value is the whole normalized + step/trigger dict, `id` included) since those are consumed by programs. + """ + trigger_pairs = _wire_pairs(live.get("triggers") or [], spec.get("triggers") or []) + step_pairs = _wire_pairs(live.get("steps") or [], spec.get("steps") or []) + + norm_live = {k: v for k, v in live.items() if k not in ("triggers", "steps")} + norm_spec = {k: v for k, v in spec.items() if k not in ("triggers", "steps")} + norm_live["triggers"] = _normalize_wire_triggers(trigger_pairs, 0) + norm_spec["triggers"] = _normalize_wire_triggers(trigger_pairs, 1) + norm_live["steps"] = _normalize_wire_steps(step_pairs, trigger_pairs, 0) + norm_spec["steps"] = _normalize_wire_steps(step_pairs, trigger_pairs, 1) + + return [ + {"path": p, "before": a, "after": b} + for p, a, b in _diff(norm_live, norm_spec, "") + ] diff --git a/tests/test_automation_payloads.py b/tests/test_automation_payloads.py index 5dc009f..71637b6 100644 --- a/tests/test_automation_payloads.py +++ b/tests/test_automation_payloads.py @@ -13,6 +13,7 @@ from kizen_builder.tools.planners.automations import ( LiveContext, _build_automation_payload, + diff_automation, plan_create_automation, plan_update_automation, ) @@ -1432,6 +1433,137 @@ def test_plan_create_omitted_active_defaults_false(patch_live_lookups): assert op.preview["active"] is False +# --------------------------------------------------------------------------- +# diff_automation: live vs. spec-as-applied, no write +# --------------------------------------------------------------------------- + + +def test_diff_automation_rejects_unknown_api_name(patch_live_lookups): + """Same lookup, same error text as `plan_update_automation` — reusing + that check rather than inventing a new error message.""" + spec = {"api_name": "no_such_auto", "name": "Nope", "type": "global", "steps": []} + with pytest.raises(PlanError, match="no automation with api_name"): + diff_automation(spec) + + +def test_diff_automation_reproduced_spec_is_empty(patch_live_lookups): + """The golden case: a spec that reproduces `test_two_code_steps`'s live + shape — hand-authored keys, real ids echoed back, `active` omitted — + diffs to nothing, even though the fixture's live steps use + `live_to_payload`-synthesized keys and this spec uses different ones.""" + raw = load_fixture("automations/two_code_steps.raw.json") + steps = sorted(raw["steps"], key=lambda s: s["order"]) + spec = { + "api_name": "test_two_code_steps", + "name": "Test Two Code Steps", + "type": "global", + # `active` deliberately omitted — resolves to live's `false` via + # BCLI-016, so this also proves that resolution reaches `diff`. + "triggers": [ + { + "trigger_type": "manual", + "order": 0, + "id": raw["triggers"][0]["id"], + "description": raw["triggers"][0]["description"], + } + ], + "steps": [ + { + "key": "step0", + "id": steps[0]["id"], + "step_type": "code_step", + "order": 0, + "parent_key": None, + "description": steps[0]["description"], + "user_description": steps[0]["user_description"], + "action_on_failure": "notify_pause", + "action_code_step": steps[0]["action_code_step"], + }, + { + "key": "step1", + "id": steps[1]["id"], + "step_type": "code_step", + "order": 1, + "parent_key": "step0", + "description": steps[1]["description"], + "user_description": steps[1]["user_description"], + "action_on_failure": "notify_pause", + "action_code_step": steps[1]["action_code_step"], + }, + ], + } + result = diff_automation(spec) + assert result["api_name"] == "test_two_code_steps" + assert result["revision"] == raw["revision"] + assert result["diff"] == [] + + +def test_diff_automation_surfaces_active_flip(patch_live_lookups): + """`active` is diffed like any other top-level field — no special-casing + for the BCLI-016 bug beyond reusing `plan_update_automation`'s own + resolution: an *explicit* flip in the spec still shows.""" + raw = load_fixture("automations/two_code_steps.raw.json") + steps = sorted(raw["steps"], key=lambda s: s["order"]) + spec = { + "api_name": "test_two_code_steps", + "name": "Test Two Code Steps", + "type": "global", + "active": True, # live is false + "triggers": [ + { + "trigger_type": "manual", + "order": 0, + "id": raw["triggers"][0]["id"], + "description": raw["triggers"][0]["description"], + } + ], + "steps": [ + { + "key": "step0", + "id": steps[0]["id"], + "step_type": "code_step", + "order": 0, + "parent_key": None, + "description": steps[0]["description"], + "user_description": steps[0]["user_description"], + "action_on_failure": "notify_pause", + "action_code_step": steps[0]["action_code_step"], + }, + { + "key": "step1", + "id": steps[1]["id"], + "step_type": "code_step", + "order": 1, + "parent_key": "step0", + "description": steps[1]["description"], + "user_description": steps[1]["user_description"], + "action_on_failure": "notify_pause", + "action_code_step": steps[1]["action_code_step"], + }, + ], + } + result = diff_automation(spec) + assert result["diff"] == [{"path": "active", "before": False, "after": True}] + + +def test_diff_automation_never_writes(patch_live_lookups, monkeypatch): + """Constraint check: `diff_automation` must not go anywhere near + `update_automation` (the PUT). Fails loudly if it ever does.""" + from kizen_builder.api import automations as auto_api + + def _boom(*args, **kwargs): + raise AssertionError("diff_automation must never call update_automation") + + monkeypatch.setattr(auto_api, "update_automation", _boom) + spec = { + "api_name": "test_two_code_steps", + "name": "Test Two Code Steps", + "type": "global", + "steps": [], + } + diff_automation(spec) # steps=[] means every live step reports as removed + + # --------------------------------------------------------------------------- # condition filter_config: JSON spec rendering + raw normalization # --------------------------------------------------------------------------- diff --git a/tests/test_cli.py b/tests/test_cli.py index 2be5103..06e3193 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1632,6 +1632,90 @@ def fake_planner(spec): assert json.loads(result.stdout)["summary"] == "test plan" +def test_automations_diff_reads_spec_from_stdin(monkeypatch): + seen = {} + + def fake_diff(spec): + seen["spec"] = spec + return { + "env": "testenv", + "api_name": "x", + "id": "auto-1", + "revision": 4, + "diff": [], + } + + monkeypatch.setattr(auto_planners, "diff_automation", fake_diff) + spec = {"api_name": "x", "name": "X", "type": "global", "steps": []} + result = runner.invoke( + cli.app, ["automations", "diff", "x"], input=json.dumps(spec) + ) + assert result.exit_code == 0, result.output + assert seen["spec"] == spec + assert "no changes" in result.output + + +def test_automations_diff_json_emits_full_result(monkeypatch): + fake_result = { + "env": "testenv", + "api_name": "x", + "id": "auto-1", + "revision": 4, + "diff": [ + { + "path": "steps.76af48bd.action_code_step.script", + "before": "a", + "after": "b", + } + ], + } + monkeypatch.setattr(auto_planners, "diff_automation", lambda spec: fake_result) + spec = json.dumps({"api_name": "x", "name": "X", "type": "global", "steps": []}) + result = runner.invoke(cli.app, ["automations", "diff", "x", "--json"], input=spec) + assert result.exit_code == 0, result.output + assert json.loads(result.stdout) == fake_result + + +def test_automations_diff_renders_change_lines(monkeypatch): + fake_result = { + "env": "testenv", + "api_name": "x", + "id": "auto-1", + "revision": 4, + "diff": [ + { + "path": "steps.76af48bd.action_code_step.script", + "before": "a", + "after": "b", + } + ], + } + monkeypatch.setattr(auto_planners, "diff_automation", lambda spec: fake_result) + spec = json.dumps({"api_name": "x", "name": "X", "type": "global", "steps": []}) + result = runner.invoke(cli.app, ["automations", "diff", "x"], input=spec) + assert result.exit_code == 0, result.output + assert "1 change" in result.output + assert "steps.76af48bd.action_code_step.script" in result.output + + +def test_automations_diff_rejects_mismatched_api_name(monkeypatch): + spec = json.dumps({"api_name": "x", "name": "X", "type": "global", "steps": []}) + result = runner.invoke(cli.app, ["automations", "diff", "not-x"], input=spec) + assert result.exit_code == 2 + assert "does not match" in result.stderr + + +def test_automations_diff_propagates_plan_error(monkeypatch): + def fake_diff(spec): + raise auto_planners.PlanError("no automation with api_name 'x'") + + monkeypatch.setattr(auto_planners, "diff_automation", fake_diff) + spec = json.dumps({"api_name": "x", "name": "X", "type": "global", "steps": []}) + result = runner.invoke(cli.app, ["automations", "diff", "x"], input=spec) + assert result.exit_code == 1 + assert "no automation with api_name" in result.stderr + + def test_plan_star_commands_are_gone(): result = runner.invoke(cli.app, ["plan-create-field", "invoice"]) assert result.exit_code != 0 diff --git a/tests/test_translate.py b/tests/test_translate.py index e4e77b8..15dad3d 100644 --- a/tests/test_translate.py +++ b/tests/test_translate.py @@ -11,11 +11,13 @@ from __future__ import annotations +import copy import json import pytest from kizen_builder.translate import ( + diff_wire_payloads, live_to_payload, semantic_diff, synthesize_step_keys, @@ -245,3 +247,205 @@ def test_message_steps_reference_message_by_id(kitchen: dict) -> None: assert set(email["email"]) == {"id"} text = _step_block(kitchen, "send_related_contact_text") assert set(text["text"]) == {"id"} + + +# --------------------------------------------------------------------------- +# diff_wire_payloads: live vs. spec-as-applied, wire (PUT) dialect +# --------------------------------------------------------------------------- + + +def _rekey(payload: dict, prefix: str) -> dict: + """Return a deep copy with every step/trigger `key` replaced by a + differently-named one, `parent_key` and `go_to_automation_step` + references remapped to match — the shape a hand-authored spec takes + (author picks their own keys; identity rides on `id`, not `key`). + Simulates the exact churn `key`/`parent_key` exclusion exists to absorb, + including a `go_to` pointed at the same target under its new key.""" + out = copy.deepcopy(payload) + step_map = {s["key"]: f"{prefix}step{i}" for i, s in enumerate(out["steps"])} + trigger_map = { + t["key"]: f"{prefix}trigger{i}" for i, t in enumerate(out["triggers"]) + } + for s in out["steps"]: + s["key"] = step_map[s["key"]] + if s["parent_key"]: + s["parent_key"] = step_map[s["parent_key"]] + go_to = s.get("action_go_to_automation_step") + if isinstance(go_to, dict): + if go_to.get("step_key"): + go_to["step_key"] = step_map[go_to["step_key"]] + if go_to.get("trigger_key"): + go_to["trigger_key"] = trigger_map[go_to["trigger_key"]] + for t in out["triggers"]: + t["key"] = trigger_map[t["key"]] + return out + + +def test_diff_wire_payloads_self_diff_is_empty() -> None: + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = live_to_payload(raw) + assert diff_wire_payloads(live, spec) == [] + + +def test_diff_wire_payloads_ignores_key_resynthesis() -> None: + """The golden case this item exists for: a spec that reproduces a live + automation, authored with its own keys, must show zero diff even though + every `key`/`parent_key` differs textually from the live side's + synthesized ones.""" + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = _rekey(live_to_payload(raw), "authored_") + assert diff_wire_payloads(live, spec) == [] + + +def test_diff_wire_payloads_ignores_go_to_key_resynthesis() -> None: + """A `go_to_automation_step` reference must resolve by matched identity, + same as `parent_key` — re-keying it to point at the *same* target under + its new key must not register as a change.""" + raw = load_fixture("automations/on_or_around_date_goto.raw.json") + live = live_to_payload(raw) + spec = _rekey(live_to_payload(raw), "authored_") + assert diff_wire_payloads(live, spec) == [] + + +def test_diff_wire_payloads_reports_go_to_retarget() -> None: + """A go_to genuinely pointed at a different step must still surface — + matched-identity resolution must not suppress a real retarget.""" + raw = load_fixture("automations/on_or_around_date_goto.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + go_to_step = next(s for s in spec["steps"] if s["type"] == "go_to_automation_step") + other_step = next( + s + for s in spec["steps"] + if s["key"] != go_to_step["action_go_to_automation_step"]["step_key"] + and s["type"] != "go_to_automation_step" + ) + go_to_step["action_go_to_automation_step"]["step_key"] = other_step["key"] + entries = diff_wire_payloads(live, spec) + assert len(entries) == 1 + (entry,) = entries + assert entry["path"].endswith(".action_go_to_automation_step.step_key") + assert entry["before"] != entry["after"] + assert entry["after"] == other_step["id"][:8] + + +def test_diff_wire_payloads_reports_added_step() -> None: + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + spec["steps"].append( + { + "key": "s02_stop_execution", + "parent_key": spec["steps"][-1]["key"], + "parent_yes_no": "", + "parent_condition": "", + "type": "stop_execution", + "prefix": "step", + "order": 2, + "user_description": "", + "action_on_failure": "notify_continue", + "should_skip_execution": False, + "goal_type": False, + "action_stop_execution": {}, + } + ) + entries = diff_wire_payloads(live, spec) + assert len(entries) == 1 + (entry,) = entries + assert entry["before"] == "" + assert entry["after"]["type"] == "stop_execution" + assert entry["path"].startswith("steps.new:") + + +def test_diff_wire_payloads_reports_removed_step() -> None: + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + removed = spec["steps"].pop() + entries = diff_wire_payloads(live, spec) + assert len(entries) == 1 + (entry,) = entries + assert entry["after"] == "" + assert entry["before"]["id"] == removed["id"] + assert entry["path"] == f"steps.{removed['id'][:8]}" + + +def test_diff_wire_payloads_dangling_spec_id_is_addition_not_edit() -> None: + """A spec step carrying an `id` that matches no live step must be an + addition, and the live step it displaces a removal — not merged into a + single "step edited" entry, which would misreport a delete+add as an + in-place change and hide the spec's bogus id entirely.""" + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + removed = spec["steps"].pop() + spec["steps"].append( + { + "id": "99999999-9999-9999-9999-999999999999", + "key": "s01_stop_execution", + "parent_key": spec["steps"][-1]["key"], + "parent_yes_no": "", + "parent_condition": "", + "type": "stop_execution", + "prefix": "step", + "order": 1, + "user_description": "", + "action_on_failure": "notify_continue", + "should_skip_execution": False, + "goal_type": False, + "action_stop_execution": {}, + } + ) + entries = diff_wire_payloads(live, spec) + by_path = {e["path"]: e for e in entries} + assert by_path[f"steps.{removed['id'][:8]}"]["after"] == "" + assert by_path["steps.99999999"]["before"] == "" + assert ( + by_path["steps.99999999"]["after"]["id"] + == "99999999-9999-9999-9999-999999999999" + ) + assert len(entries) == 2 + + +def test_diff_wire_payloads_reports_one_changed_field() -> None: + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + spec["steps"][0]["action_code_step"]["script"] = 'outputs.log("changed")' + entries = diff_wire_payloads(live, spec) + assert len(entries) == 1 + (entry,) = entries + step_octet = live["steps"][0]["id"][:8] + assert entry["path"] == f"steps.{step_octet}.action_code_step.script" + assert entry["before"] == 'outputs.log("step 1")' + assert entry["after"] == 'outputs.log("changed")' + + +def test_diff_wire_payloads_reports_reparenting() -> None: + """Excluding `parent_key` from the literal comparison must not also hide + a genuine reparenting — the parent is compared by matched identity.""" + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + spec["steps"][1]["parent_key"] = None + entries = diff_wire_payloads(live, spec) + assert len(entries) == 1 + (entry,) = entries + child_octet = live["steps"][1]["id"][:8] + parent_octet = live["steps"][0]["id"][:8] + assert entry["path"] == f"steps.{child_octet}.parent" + assert entry["before"] == parent_octet + assert entry["after"] is None + + +def test_diff_wire_payloads_active_is_diffed_like_any_top_level_field() -> None: + raw = load_fixture("automations/two_code_steps.raw.json") + live = live_to_payload(raw) + spec = copy.deepcopy(live) + spec["active"] = not live["active"] + entries = diff_wire_payloads(live, spec) + assert entries == [ + {"path": "active", "before": live["active"], "after": spec["active"]} + ] From b5cf5882f4d90453f8d12867c75ca2621752dcb3 Mon Sep 17 00:00:00 2001 From: Jeremy Bedient Date: Thu, 13 Aug 2026 15:09:42 -0400 Subject: [PATCH 2/2] Stop claiming the wire diff shares roundtrip's drift path convention `diff_wire_payloads`'s docstring said its return value was "the same shape `roundtrip_automation`'s `drift` field already uses." The three keys match; the `path` convention does not. `drift` comes from `semantic_diff`, which identifies steps positionally (`steps[3].field`) and carries no id anywhere, so it is not a precedent for these id-octet-labelled paths. --- src/kizen_builder/translate.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/kizen_builder/translate.py b/src/kizen_builder/translate.py index 14f88f5..c61b6bb 100644 --- a/src/kizen_builder/translate.py +++ b/src/kizen_builder/translate.py @@ -669,8 +669,10 @@ def diff_wire_payloads( `go_to_automation_step` reference is resolved to its target's matched identity the same way `parent_key` is, so it survives key resynthesis too. - Returns ``[{"path", "before", "after"}, ...]`` — the same shape - `roundtrip_automation`'s `drift` field already uses. A step/trigger only + Returns ``[{"path", "before", "after"}, ...]`` — the same three keys + `roundtrip_automation`'s `drift` field uses, but *not* the same `path` + convention: `drift` comes from :func:`semantic_diff` and numbers steps + positionally (`steps[3].field`), carrying no id at all. A step/trigger only on one side (`before` or `after` is the literal `""` sentinel) is an addition or removal; anything else is a changed field. `path` embeds each step/trigger's first-id-octet label so a line can be matched to the