Cli onbehaflof

[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:	9e5857c
Status:	✅  Deploy successful!
Preview URL:	https://00f08363.windmill.pages.dev
Branch Preview URL:	https://cli-onbehaflof.windmill.pages.dev

View logs