Skip to content

Re-land automations diff - #25

Merged
jbedient-kizen merged 2 commits into
mainfrom
feat/reland-automations-diff
Sep 1, 2026
Merged

Re-land automations diff#25
jbedient-kizen merged 2 commits into
mainfrom
feat/reland-automations-diff

Conversation

@jbedient-kizen

@jbedient-kizenjbedient-kizen commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

This adds no new work.kizen automations diff <api_name> --spec-file <path> was built, reviewed, and merged as PR #12 — and it is not on main.

What happened

Same root cause as #24. PR #12 was merged into bug/automation-update-deactivates-without-asking rather than main, because that item hadn't merged yet. It then merged to main and its branch was deleted, so #12's content — which lived only on top of that branch — never propagated. GitHub records #12 as MERGED.

Verified: git merge-base --is-ancestor 6c104316 main returns non-ancestor, main's cli/automations.py has no diff subcommand, and main's scripts/cli-tree-baseline.txt has no entry for it.

Why this one is a rebase, not a merge

Unlike #24, this branch does not merge cleanly. It was cut in mid-August and cli/automations.py has since taken PR #20's merge_fields integration. A two-dot diff against the branch shows ~6,900 deletions purely because the branch lacks everything main gained since — merging it would drag that staleness in as a resolution exercise across ~63 files.

So the two commits were cherry-picked onto current main instead, which keeps the reviewable diff to just the feature.

The two conflicts, and how they were resolved

Both were benign and are worth stating explicitly so a reviewer can check the judgement rather than take it on trust:

  1. src/kizen_builder/cli/automations.py — both sides added an import at the same location (KizenAPIError on main, _read_spec on the branch). Kept both; confirmed both are actually used (:16, :17, used at :515 and :310).
  2. CHANGELOG.md — competing additive entries in [Unreleased]. Kept both blocks in full (49 lines from main, 15 from the branch). Nothing dropped.

scripts/cli-tree-baseline.txt auto-merged and passes extra_checks unmodified.

Verification

bin/check.sh builder-cli --dir <worktree> — lint, format, typecheck, test, extra_checks all pass:

1269 passed, 4 skipped, 76 deselected in 18.61s

main's baseline is 1250 passed, so all 19 tests these commits add are present and passing, including the tests/test_translate.py and tests/test_cli.py additions.

The specific regression risk here was that a careless resolution would silently revert main's merge_fields work. Confirmed intact: tools/planners/automations.py still imports merge_fields and still calls merge_fields.render at all three sites (:1669, :1741, :2102). uv run kizen automations diff --help renders, and automations diff is present in the regenerated CLI tree.

Two known caveats — neither introduced here, both worth reading before merge

  1. diff shares roundtrip's client-side-only limitation. Validation is client-side; the mismatches that matter are server-side. A clean diff does not prove a subsequent update would be accepted. The second commit here (Stop claiming the wire diff shares roundtrip's drift path convention) already walks back one overclaim of this kind. The general fix is tracked separately, along with making roundtrip state what it did not check.

  2. diff does not report phantom drift on notify steps. An earlier note predicted it would — that prediction is disproven, and is recorded here because it would otherwise lead a reviewer to distrust a non-empty diff for the wrong reason.

    The prediction was reasonable: the spec→payload builders for notify_member_via_text / _via_email assemble from a key allowlist (team_member/content/html_content/base_message_id and team_member/cc_team_member/id) that never reads name. But diff's live side is not the raw GET — translate.live_to_payload runs each live step through that same _STEP_BUILDERS registry (translate.py:38, used at :179). Both sides therefore drop name identically, it is absent on both, and the symmetric key walk in _diff emits nothing:

    live side keys: ['base_message_id', 'content', 'html_content', 'team_member']
    spec side keys: ['base_message_id', 'content', 'html_content', 'team_member']
    diff between the two sides: []
    

    The server's own schema confirms dropping name is correct rather than lossy: ActionNotifyMemberViaText (read) requires name, while ActionNotifyMemberViaTextRequest (write) has no such property — base_message_id, content, html_content, team_member only. Same for WriteActionNotifyMemberViaEmailRequest. It is read-only server state, so the write dialect has no way to carry it.

    What remains real, and is not this PR's surface: roundtrip --execute performs a write, and because the PUT cannot carry name, the server blanks it (observed as "…Version 3" → ""). That is a pre-existing write-side limitation of roundtrip, tracked separately, and unaffected by anything here. A related cleanup is that both notify config models use extra="allow", so a name written into a hand-authored spec validates and is then discarded — accepting a key and ignoring it is the part worth fixing.

`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.
`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.
@jbedient-kizen
jbedient-kizenforce-pushed the feat/reland-automations-diff branch from 35b9dc6 to b5cf588CompareSeptember 1, 2026 16:30
@jbedient-kizen
jbedient-kizen merged commit 060d4bb into mainSep 1, 2026
4 checks passed
@jbedient-kizen
jbedient-kizen deleted the feat/reland-automations-diff branch September 4, 2026 16:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jbedient-kizen@annaliu-kizen