feat: rules for preserving and setting on_behalf_of from the cli

[wendrul]

No description provided.

[wendrul]

/ai-fast review

[claude]

Claude finished @wendrul's task in 2m 1s —— View job

---

PR Review: CLI on-behalf-of

• Read all changed files
• Analyze code quality, bugs, and patterns
• Post review feedback

---

Summary

This PR adds permissioned_as / on_behalf_of support to the Windmill CLI sync push workflow. It allows admins/deployers to preserve ownership when syncing resources, and supports defaultPermissionedAs rules in wmill.yaml for assigning ownership on new items via glob patterns. The feature is gated behind respectVirtualUserPermissions (default true).

The implementation is well-structured with good separation of concerns (new permissioned_as.ts module), comprehensive validation, and solid test coverage for the rule engine.

---

Issues

Bug: lookupUsernameByEmail cache logic is fragile

cli/src/core/permissioned_as.ts:116

if (cache.size === 0) {
  const users = await wmill.listUsers({ workspace });
  for (const user of users) {
    cache.set(user.email, user.username);
  }
}

The cache population check cache.size === 0 only works correctly on the first call. If the first call populates the cache but the desired email is not found, subsequent calls with different emails will never re-fetch because the cache is now non-empty. This is by design (fetch once), but it means:

• If a user is added to the workspace between CLI invocations within the same sync run, they won't be found
• More importantly, if the first email lookup succeeds (populating the cache), but a second email doesn't exist in the workspace, you get a clear error — this is fine

However, the pattern is fragile because the cache population is hidden inside the lookup function. Consider making the "populate cache" step explicit and separate from the lookup.

Bug: whoami output change may break scripts

cli/src/commands/workspace/workspace.ts:411

-  log.info(await wmill.globalWhoami());
+  log.info(Object.entries(await wmill.globalWhoami()));

Object.entries() returns an array of [key, value] pairs. When logged, this produces output like login_type,password,email,admin@windmill.dev,... which is much harder to read than the previous object output. Was this intentional? If the goal is better formatting, consider JSON.stringify(await wmill.globalWhoami(), null, 2) instead.

Fix this →

Potential Issue: preCheckPermissionedAs regex matching is brittle

cli/src/core/permissioned_as.ts:184-189

const match = beforeContent.match(
  /on_behalf_of_email:\s*["']?([^\s"']+)["']?/
);

This regex-based YAML parsing is fragile. If the YAML value is quoted with special characters or has trailing comments, it may not extract the value correctly. Consider using the existing yamlParseContent utility for more robust parsing. Same issue applies to the schedule email regex at line 207-209.

Inconsistency: Schedule and Trigger preserve_email vs Script/Flow/App preserve_on_behalf_of

The preserve field names are inconsistent across resource types:

• Scripts, Flows, Apps use preserve_on_behalf_of
• Schedules and Triggers use preserve_email

This appears to match the backend API, but it's worth documenting this inconsistency somewhere to avoid confusion for future maintainers.

Missing: preCheckPermissionedAs doesn't handle scripts well

cli/src/core/permissioned_as.ts:178-189

For scripts, preCheckPermissionedAs only looks at .script.yaml / .script.json files. But changes may contain the actual script content file (.ts, .py, etc.) rather than the metadata file. This means edited scripts might not trigger the ownership warning for non-admin users.

Typo: respectVirtualUserPermissions default is true

cli/src/core/conf.ts:380

respectVirtualUserPermissions: true,

This means the feature is enabled by default, which is a behavior change. Existing users who aren't admins/deployers will now get prompted (or blocked in CI) when pushing items owned by others. This should probably be called out prominently in release notes. The backwards compat commit (9e5857c) seems related — where is this flag?

Minor Issues

1. cli/src/commands/app/app.ts:203 — Debug log line is very long. Consider breaking it up or using structured logging.

2. cli/src/core/permissioned_as.ts:256-258 — In non-interactive mode, process.exit(1) is called. This is fine for the CLI but makes testing harder. Consider throwing an error instead and letting the caller handle the exit.

3. cli/src/commands/schedule/schedule.ts:126-129 — The schedule preserve logic uses edited_by from the remote schedule as the email field. Verify this is what the backend expects (a username, not an email).

4. PR title typo: "onbehaflof" → "on_behalf_of"

Strengths

• Clean module structure with permissioned_as.ts as a standalone module
• Good test coverage for rule validation and resolution
• Consistent pattern across all resource types (scripts, flows, apps, schedules, triggers)
• Pre-check warning system for non-admin users is a nice UX touch
• The defaultPermissionedAs glob-based rules in wmill.yaml is a well-designed configuration approach

---

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	30c632a
Status:	✅  Deploy successful!
Preview URL:	https://15058376.windmill.pages.dev
Branch Preview URL:	https://cli-onbehaflof.windmill.pages.dev

View logs

[claude]

Claude finished @wendrul's task in 2m 51s —— View job

---

PR Review: CLI on_behalf_of support

• Read all changed files
• Analyze code quality, bugs, and patterns
• Post comprehensive review feedback

---

Summary

This PR adds permissioned_as / on_behalf_of support to the CLI sync push workflow. Key capabilities:

• Preserve ownership when admin/deployer users sync resources (scripts, flows, apps, schedules, triggers)
• Assign ownership on new items via defaultPermissionedAs glob rules in wmill.yaml
• Pre-check warning for non-admin/non-deployer users about ownership changes
• set-permissioned-as CLI subcommands for each resource type
• Strip on_behalf_of_email from pulled metadata files (version >= 0.1)
• targetWorkspace support in getEffectiveSettings for workspace-specific rule resolution

The implementation is well-structured with a clean permissioned_as.ts module, good test coverage for validation/resolution, and consistent patterns across resource types.

---

Issues

Bug: preCheckPermissionedAs type mismatch with sync changes

cli/src/core/permissioned_as.ts:133-139 and cli/src/commands/sync/sync.ts:2606-2612

The Change interface in permissioned_as.ts defines before?: string, but the sync changes array uses the Edit type which has before: string (non-optional) plus a codebase?: string field. While these are structurally compatible at runtime, the preCheckPermissionedAs function accepts Change[] but is passed the broader sync changes union type without an explicit cast. TypeScript should catch this if the types diverge further. Consider importing and using the actual sync change types, or at minimum adding a comment explaining the structural typing dependency.

Bug: lookupUsernameByEmail cache won't re-fetch after initial population

cli/src/core/permissioned_as.ts:116

if (cache.size === 0) {
  const users = await wmill.listUsers({ workspace });
  ...
}

The cache.size === 0 check means the user list is only fetched once (first call). This is fine for performance but is fragile — if the initial fetch returns users but a later rule references an email added after that fetch, it will fail with a confusing error. Consider adding a comment documenting this "fetch-once" behavior, or making the cache population an explicit separate step called before the push loop begins (e.g., in the sync push function right after building permissionedAsContext).

Fix this →

Bug: preCheckPermissionedAs regex-based YAML parsing is fragile

cli/src/core/permissioned_as.ts:184-189

const match = beforeContent.match(
  /on_behalf_of_email:\s*["']?([^\s"']+)["']?/
);

This regex won't correctly handle multi-line YAML values, YAML block scalars, or values with special characters. The beforeContent here comes from the pre-existing remote content string, not a parsed YAML file. Since this is for a warning only (not for correctness-critical logic), it's acceptable, but it would be more robust to use the existing yamlParseContent utility. Same applies to the schedule email regex at line 207-209.

Potential Issue: on_behalf_of_email stripped during pull may cause data loss on round-trip

cli/src/commands/sync/sync.ts:629-631 and cli/src/commands/sync/sync.ts:905-907

if (stripOnBehalfOf) {
  delete (flow as any).on_behalf_of_email;
}

When version >= 0.1, pulling strips on_behalf_of_email from flows and script metadata. Then on push, if the user is an admin/deployer, the code fetches the remote's current on_behalf_of_email and sets preserve_on_behalf_of: true. This round-trip works correctly only if the remote still has the same owner. If someone changes ownership via the UI between pull and push, the preserve logic will correctly use the current remote value. Good design.

However, the stripping is unconditional for version >= 0.1. If someone intentionally sets on_behalf_of_email in their local YAML file (to explicitly assign ownership in git), that value will be pulled away. This seems intentional but should be documented.

Style: Stale comment references respectVirtualUserPermissions

cli/src/commands/sync/sync.ts:2587

// Build permissioned_as context (only when respectVirtualUserPermissions is enabled)

The actual gating is version >= 0.1, not respectVirtualUserPermissions. This comment is stale/misleading.

Fix this →

Potential Issue: Script set-permissioned-as creates a new version

cli/src/commands/script/script.ts:1309-1355

The setPermissionedAs function for scripts creates a new script version (via createScript) rather than updating the existing one in-place. This is because the Windmill API for scripts is versioned (immutable). This is correct API usage, but the user should be aware that running wmill script set-permissioned-as will create a new version of the script. The parent_hash is set correctly to chain versions. Good.

However, the function copies only a subset of fields from the remote script. If the remote script has additional fields like envs, ws_error_handler_muted, dedicated_worker, concurrent_limit, concurrency_time_window_s, cache_ttl, priority, restart_unless_cancelled, timeout, delete_after_use, visible_to_runner_only, or no_main_func, these will be silently dropped. This could cause unintended behavior changes when setting permissioned_as.

Fix this →

Inconsistency: Schedule preserve_email uses edited_by (username) not email

cli/src/commands/schedule/schedule.ts:127-129

if ((schedule as Schedule).edited_by) {
  preserveFields.email = (schedule as Schedule).edited_by;

For schedules, the email field in preserve is set to edited_by, which is a username, not an email. The naming is confusing (email field receives a username value). This matches the backend API's expectation for schedules, but it's worth adding a comment explaining this. Same pattern in triggers at trigger.ts:176-178.

Missing: defaultPermissionedAs not validated at branch level

cli/src/core/conf.ts:566-568 and cli/src/core/conf.ts:592-594

if (gitBranches[currentBranch].defaultPermissionedAs) {
  effective.defaultPermissionedAs = gitBranches[currentBranch].defaultPermissionedAs;
}

defaultPermissionedAs is read from branch-level config and assigned directly to effective without validation. The validation only happens later in sync.ts:2594 via validatePermissionedAsRules. This means a typo in a branch-level rule (e.g., emall instead of email) won't be caught until push time. Consider validating during getEffectiveSettings or readConfigFile.

Minor Issues

1. cli/src/core/permissioned_as.ts:256-258 — process.exit(1) in preCheckPermissionedAs makes testing harder. Consider throwing a custom error and letting the caller handle the exit.

2. cli/src/commands/sync/sync.ts:2589 — parseFloat(opts.version ?? "0") >= 0.1 — using parseFloat for version comparison is fragile (e.g., 0.10 vs 0.1). Consider using a proper semver comparison if versions become more complex, or at minimum adding a comment that this only works for simple decimal versions.

3. cli/src/core/permissioned_as.ts:19 — KNOWN_RULE_FIELDS could be derived from the PermissionedAsRule interface type keys to stay in sync automatically (using a validation library or manual const assertion).

Strengths

• Clean module structure with permissioned_as.ts as a standalone, well-tested module
• Good test coverage: 271 lines of unit tests for rule validation/resolution, 148 lines for target workspace config resolution
• Consistent preserve pattern across all resource types
• Pre-check warning system for non-admin users is a nice UX addition
• Glob-based defaultPermissionedAs rules are a well-designed configuration approach
• Stripping on_behalf_of_email during pull prevents accidental ownership drift in git

---

Testing Instructions

To test these changes, you need a Windmill instance with at least two users (one admin, one non-admin). Navigate to a workspace with existing scripts, flows, apps, schedules, and triggers. Set up a wmill.yaml with version: "0.1" and defaultPermissionedAs rules mapping glob patterns to user emails. Run wmill sync push as an admin user — verify that existing items preserve their ownership and new items get assigned based on the rules. Then run as a non-admin user and verify the pre-check warning appears, listing items whose ownership would change. Test the set-permissioned-as subcommands (e.g., wmill script set-permissioned-as <path> <email>) and verify the ownership change is reflected in the Windmill UI on the script/flow/app/schedule/trigger detail pages. Also verify wmill sync pull strips on_behalf_of_email from YAML files when version >= 0.1.

---