Skip to content

fix(envvars): restore workflowUserId fallback for scheduled execution env var resolution#3941

Merged
waleedlatif1 merged 2 commits intostagingfrom
waleedlatif1/fix-env-var-schedule
Apr 4, 2026
Merged

fix(envvars): restore workflowUserId fallback for scheduled execution env var resolution#3941
waleedlatif1 merged 2 commits intostagingfrom
waleedlatif1/fix-env-var-schedule

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Apr 4, 2026 6:09pm

Request Review

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 4, 2026

PR Summary

Medium Risk
Changes which user’s personal secrets are loaded during execution, affecting scheduled/webhook runs and potentially altering credential access if metadata is wrong.

Overview
Restores env-var lookup scoping in executeWorkflowCore so client sessions resolve personal env vars via sessionUserId, while server-side executions (e.g. schedules/webhooks) resolve via workflowUserId instead of the billing/actor userId.

Adds coverage ensuring the correct user id is passed to getPersonalAndWorkspaceEnv, and hard-fails with a clearer error when workflowUserId is missing for server-side runs.

Reviewed by Cursor Bugbot for commit 0bcc774. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 4, 2026

Greptile Summary

This PR fixes a regression introduced in #3179 where scheduled workflow executions resolved personal env vars against the billing/actor user instead of the workflow creator. The fix restores the original two-path logic from #2197: client sessions (manual runs from the UI) use sessionUserId, while server-side executions (schedules, webhooks, API triggers) use workflowUserId. The || metadata.userId safety net that silently resolved against the wrong user is intentionally removed in favour of an explicit throw, closing the silent-misbehaviour window.

Changes:

  • execution-core.ts: personalEnvUserId now uses a guarded ternary instead of the regressed sessionUserId || userId short-circuit, and throws a clearer error when neither applicable ID is present.
  • execution-core.test.ts: workflowUserId is added to the base createSnapshot() fixture, and three new tests cover all three branches (client session, server-side, and missing-workflowUserId error path) — directly addressing the coverage gap raised in the previous review thread.

Confidence Score: 5/5

Safe to merge — the bug fix is correct, all three branches are tested, and the only remaining finding is a P2 improvement to an error message.

All three code paths in the restored logic are covered by new unit tests. The fix precisely targets the regression (wrong user for scheduled env-var resolution) without touching unrelated behaviour. The sole open comment is a P2 cosmetic improvement to the error message for an already-unusual edge case (client session with no sessionUserId).

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/workflows/executor/execution-core.ts Replaces `sessionUserId
apps/sim/lib/workflows/executor/execution-core.test.ts Adds workflowUserId to the base snapshot fixture and three new tests covering all branches of the restored env-var resolution logic.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[executeWorkflowCore] --> B{metadata.isClientSession\n&& metadata.sessionUserId?}
    B -- Yes --> C[personalEnvUserId = sessionUserId\nclient / manual run]
    B -- No --> D{metadata.workflowUserId?}
    D -- Set --> E[personalEnvUserId = workflowUserId\nschedule / webhook / API]
    D -- Missing --> F[throw Error:\n'Missing workflowUserId in execution metadata']
    C --> G[getPersonalAndWorkspaceEnv\npersonalEnvUserId, workspaceId]
    E --> G
    G --> H[Decrypt env vars\nand continue execution]
Loading

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Reviews (2): Last reviewed commit: "test(envvars): add coverage for env var ..." | Re-trigger Greptile

@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/fix-env-var-schedule branch from 0fa571e to 0fe6960 Compare April 4, 2026 18:04
@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/fix-env-var-schedule branch from 0fe6960 to ff77546 Compare April 4, 2026 18:08
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 0bcc774. Configure here.

@waleedlatif1 waleedlatif1 merged commit 893e322 into staging Apr 4, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/fix-env-var-schedule branch April 4, 2026 18:22
waleedlatif1 added a commit that referenced this pull request Apr 4, 2026
… env var resolution (#3941)

* fix(envvars): restore workflowUserId fallback for scheduled execution env var resolution

* test(envvars): add coverage for env var user resolution branches
Sign up for free to 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.

1 participant