fix(cli): stop spurious has_on_behalf_of/has_permissioned_as diffs on pull (WIN-2201)

[rubenfiszel]

Problem

wmill sync pull (with syncBehavior: v1) produced a spurious has_on_behalf_of: false line in the metadata of nearly every script and flow, causing endless noise in git diffs.

Why the marker exists

With syncBehavior: v1 the pull deliberately strips on_behalf_of_email from the committed metadata — the owner's email is user-specific and committing it causes ownership churn between whoever pulls. But on the next push the CLI still needs to know whether the item had an owner so it can send preserve_on_behalf_of: true and keep the remote ownership instead of clobbering it. Once the email is stripped, a boolean marker (has_on_behalf_of for scripts/flows, has_permissioned_as for schedules/triggers) is the only residual signal of "this has an owner."

So the marker is not implicit from on_behalf_of_email once that field is gone — but it is only meaningful when true. The code wrote it unconditionally as !!<field>, so every ownerless item (the overwhelming majority) got ...: false written on pull.

Fix

Emit the marker only when an owner exists. Absence of the key already means "no owner" everywhere it's read on push (typed?.has_on_behalf_of ?? !!typed?.on_behalf_of_email → false; schedules/triggers delete the marker before push and never read it), so dropping the false line is behavior-preserving.

Applied consistently to the four pull transforms in cli/src/commands/sync/sync.ts:

• scripts — has_on_behalf_of
• flows — has_on_behalf_of
• schedules — has_permissioned_as
• triggers — has_permissioned_as

Existing files that already carry a ...: false line will show a one-time cleanup diff on the next pull (the redundant line removed), then stay stable.

Testing

• cli/test/pull_on_behalf_of_marker.test.ts — new regression test: creates an ownerless script via the API, runs sync pull with syncBehavior: v1, and asserts the pulled .script.yaml contains neither has_on_behalf_of nor on_behalf_of_email. Verified it fails on the pre-fix code (received has_on_behalf_of: false) and passes with the fix.
• bunx tsc --noEmit: no new type errors (16 pre-existing, unchanged).

🤖 Generated with Claude Code

---

Summary by cubic

Stop writing has_on_behalf_of: false and has_permissioned_as: false on wmill sync pull with syncBehavior: v1. Only emit these markers when true to remove noisy diffs for ownerless items. Fixes WIN-2201.

• Bug Fixes
  • Emit has_on_behalf_of / has_permissioned_as only when an owner exists; behavior on push is unchanged.
  • Applied to scripts, flows, schedules, and triggers; existing files may see a one-time cleanup removing redundant false lines.
  • Added a regression test to ensure ownerless scripts omit both the marker and on_behalf_of_email.

^{Written for commit ac3f2a8. Summary will update on new commits.}

[rubenfiszel]

/review

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @rubenfiszel's task in 1m 13s —— View job

---

Review

Good to merge — no blocking issues.

I traced every place the four markers are read on push to confirm the "behavior-preserving" claim holds. Absence of the key and an explicit false are treated identically at all read sites, so dropping the false line changes nothing downstream:

• scripts — cli/src/commands/script/script.ts:503: (typed as any)?.has_on_behalf_of ?? !!typed?.on_behalf_of_email → undefined ?? false → false. ✅
• flows — cli/src/commands/flow/flow.ts:233: same ?? !!on_behalf_of_email pattern. ✅
• schedules / triggers — schedule.ts:124 and trigger.ts:174 delete the has_permissioned_as marker before push and never read its value; ownership is preserved via the remote lookup. ✅
• preCheckPermissionedAs — cli/src/core/permissioned_as.ts:52,57,124,141 only ever match has_on_behalf_of:\s*(true). A false line never matched before, so nothing regresses. ✅

The fix is correct, minimal, and the comments capture exactly the non-obvious constraint (why a bare false is redundant). The one-time cleanup diff on existing files is expected and self-healing — and it doesn't spuriously trip the precheck, since the stale false line doesn't match the (true) regex.

AGENTS.md compliance

• Comments record the durable constraint (why the flag is only meaningful when true), not narration — matches the "Comments record constraints, not narration" rule. ✅
• Change stays focused on the four pull transforms; no unrelated refactoring. ✅

Test coverage

CLI/TS change with an integration-style regression test (cli/test/pull_on_behalf_of_marker.test.ts) that creates an ownerless script via the API, runs sync pull with syncBehavior: v1, and asserts the pulled .script.yaml contains neither marker nor email. This is the right smallest guard and the PR notes it was verified to fail pre-fix.

Worth noting (not blocking): the test exercises only the script transform. The flow / schedule / trigger transforms are structurally identical one-liners, so a single guard is a reasonable choice per the "ship only the tests the PR needs" guidance — I wouldn't add three more near-duplicate integration tests just for symmetry.

Manual verification: none strictly required given the automated guard. If you want extra confidence on the migration path, pull a workspace that already has committed has_on_behalf_of: false / has_permissioned_as: false lines (script, flow, schedule, and trigger) and confirm the next sync pull produces a one-time cleanup diff removing those lines, then a subsequent pull is a no-op.
· ruben/win-2201-stop-spurious-has_on_behalf_of-diffs-in-script-metadata

[github-actions]

Pi Review

Good to merge — no blocking issues.

I independently verified the behavior-preserving claim at every read site. The four transforms are correct and the reasoning in the PR body holds:

• Scripts (script.ts:503): has_on_behalf_of ?? !!on_behalf_of_email → absent key is undefined, falls through to !!undefined = false. ✅
• Flows (flow.ts:233): identical ?? pattern. ✅
• Schedules / triggers: schedule.ts:124 and trigger.ts:174 only delete the marker before push; never read its value. ✅
• preCheckPermissionedAs (permissioned_as.ts:52,57,124,141): regex matches has_on_behalf_of:\s*(true) — an absent key doesn't match the (true) group. ✅

The comments follow the AGENTS.md "Comments record constraints, not narration" rule — each states why emitting a false is harmful (spurious diff for ownerless items) in ≤4 lines, with no ephemeral references.

Test coverage

CLI/TS change with an integration-style regression test (cli/test/pull_on_behalf_of_marker.test.ts) that creates an ownerless script via the API, runs sync pull with syncBehavior: v1, and asserts the pulled YAML contains neither has_on_behalf_of nor on_behalf_of_email. The PR confirms it was verified pre-fix (failing) and post-fix (passing). This is the right smallest guard per the "ship only the tests the PR needs" guidance.

Manual verification: none strictly required given the automated guard. An optional sanity check: on a workspace with existing committed has_on_behalf_of: false / has_permissioned_as: false lines across scripts, flows, schedules, and triggers, a sync pull should produce a one-time cleanup diff removing those lines, and a subsequent pull should be a no-op.