Skip to content

improvement(platform): standardize perms, audit logging, lifecycle across admin, copilot, ui actions#3858

Merged
icecrasher321 merged 5 commits intostagingfrom
improvement/ui-action-consolidation
Mar 31, 2026
Merged

improvement(platform): standardize perms, audit logging, lifecycle across admin, copilot, ui actions#3858
icecrasher321 merged 5 commits intostagingfrom
improvement/ui-action-consolidation

Conversation

@icecrasher321
Copy link
Copy Markdown
Collaborator

Summary

Admin, Copilot, UI actions had different perms, audit logging and db query behaviour leading to a range of bugs. UI actions are treated as source of truth and extracted in helpers used across in the bodies of all 3 of these surfaces.

Examples of Bugs:

  • Webhook deployed via mothership doesn't manage lifecycle like UI deploy leading to 404s
  • Chat deploy auths are different
  • Perms are different to run/create resources via UI vs mothership
  • Admin didn't cleanup resources in the same way for workflows

Type of Change

  • Bug fix
  • Other: Code improvement

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)

@cursor
Copy link
Copy Markdown

cursor bot commented Mar 31, 2026

PR Summary

Medium Risk
Touches deployment/undeploy/delete paths for workflows, chats, and folders across UI, admin API, and copilot tooling, so regressions could impact lifecycle cleanup and permissions enforcement. Changes are largely consolidations behind shared orchestration helpers, reducing divergence but increasing blast radius if helpers are wrong.

Overview
Centralizes lifecycle operations by routing chat deploy/undeploy, workflow deploy/undeploy/version-activate, and workflow/folder deletion through shared @/lib/workflows/orchestration helpers (new performChatDeploy/performChatUndeploy, and adoption of performFullDeploy/performFullUndeploy/performActivateVersion/performDeleteWorkflow/performDeleteFolder). This removes duplicated DB/webhook/schedule/MCP cleanup logic from multiple API routes and copilot tool executors and standardizes error-to-HTTP mapping.

Standardizes permissions checks in copilot tool execution by extending ensureWorkflowAccess/ensureWorkspaceAccess to accept explicit read|write|admin levels and tightening several operations to require write or admin.

Expands audit logging: adds new audit actions/resource types for custom tools, skills, and credential rename/delete, and records audit entries for skills/tools CRUD, MCP server add/update/remove, job schedule create/update/delete/complete, workflow import/create, workflow variable updates, etc.

Also removes the copilot client SSE handler implementation files (client-sse/handlers.ts and content-blocks.ts) and updates route tests to mock the new orchestration entrypoints.

Written by Cursor Bugbot for commit 2c5a65c. Configure here.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 31, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 31, 2026 3:11am

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR introduces a new apps/sim/lib/workflows/orchestration/ layer that extracts the canonical implementations of deploy, undeploy, activate-version, delete-workflow, delete-folder, chat-deploy, and chat-undeploy into shared helper functions (performFullDeploy, performFullUndeploy, performActivateVersion, performDeleteWorkflow, performDeleteFolder, performChatDeploy, performChatUndeploy). All three surfaces — the UI API routes, the admin API, and the copilot tool executor — are updated to call these helpers rather than each implementing the logic independently, eliminating the divergent permission checks, missing webhook/MCP cleanup, and missing audit entries that caused the bugs described in the PR.

Key changes:

  • New orchestration module with fully lifecycle-managed functions (webhooks, schedules, MCP sync, audit logging, telemetry) used consistently across all surfaces
  • ensureWorkspaceAccess / ensureWorkflowAccess in the copilot executor refactored from requireWrite: boolean to level: 'read' | 'write' | 'admin'; several operations correctly elevated to 'admin'
  • Expanded audit coverage: custom tools, skills, MCP server operations, job scheduling, and workflow/folder creation now emit audit entries from the copilot path
  • Two client-side SSE files (client-sse/handlers.ts, client-sse/content-blocks.ts) deleted as dead code
  • Tests updated throughout to mock at the orchestration layer rather than the individual persistence utilities

Notable concerns:

  • templateAction now defaults to 'orphan' even when deleteTemplatesParam is absent from the workflow DELETE request, silently changing behavior for callers that skip the template-check flow
  • Admin API audit entries attribute workflow owner ID as the actor (no admin session context available), which produces misleading audit trails for admin-triggered operations
  • Every chat deployment now emits both a WORKFLOW_DEPLOYED and CHAT_DEPLOYED audit entry, doubling the audit volume compared to before
  • deleteWorkflowRecord and deleteFolderRecord in utils.ts are now dead code with no callers

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 (behavioral nuances and audit attribution), with no runtime breakage or data loss risk.

The core orchestration consolidation is correct and well-tested; all three surfaces now share identical lifecycle logic. The four issues found are: (1) a subtle templateAction behavior change for edge-case DELETE callers, (2) misleading audit attribution for admin routes (pre-existing gap made slightly worse), (3) double audit events for chat deploys (harmless but noisy), and (4) dead code. None of these cause incorrect data, security problems, or functional regressions for normal user flows.

apps/sim/app/api/workflows/[id]/route.ts (templateAction default), apps/sim/app/api/v1/admin/workflows/[id]/route.ts (audit actor), apps/sim/lib/workflows/orchestration/chat-deploy.ts (double audit events)

Important Files Changed

Filename Overview
apps/sim/lib/workflows/orchestration/deploy.ts New file: canonical performFullDeploy, performFullUndeploy, and performActivateVersion orchestration functions — consolidates previously duplicated deploy/undeploy logic across UI, admin, and copilot surfaces with full lifecycle management (webhooks, schedules, MCP sync, audit logging).
apps/sim/lib/workflows/orchestration/chat-deploy.ts New file: performChatDeploy and performChatUndeploy helpers — correctly upserts chat records and delegates to performFullDeploy, but every chat deploy now emits two audit entries (WORKFLOW_DEPLOYED + CHAT_DEPLOYED) which is a change from the previous single-event behavior.
apps/sim/lib/workflows/orchestration/workflow-lifecycle.ts New file: performDeleteWorkflow with last-workflow guard, template orphan/delete handling, and audit logging — clean consolidation; the skipLastWorkflowGuard flag correctly enables admin bypasses.
apps/sim/lib/workflows/orchestration/folder-lifecycle.ts New file: performDeleteFolder with last-workflow guard and recursive deletion helpers moved from folders/[id]/route.ts — logic is preserved faithfully and now shared across copilot and UI surfaces.
apps/sim/app/api/workflows/[id]/route.ts DELETE handler simplified to use performDeleteWorkflow; templateAction now defaults to 'orphan' even when deleteTemplatesParam is absent, silently changing behavior for requests that skip the template-check flow.
apps/sim/app/api/v1/admin/workflows/[id]/route.ts Admin workflow DELETE now calls performDeleteWorkflow with userId: workflowData.userId; audit entries will attribute admin-triggered deletions to the workflow owner rather than an admin actor.
apps/sim/lib/copilot/orchestrator/tool-executor/access.ts ensureWorkspaceAccess signature changed from requireWrite: boolean to `level: 'read'
apps/sim/lib/copilot/orchestrator/tool-executor/workflow-tools/mutations.ts Workflow/folder mutations unified with orchestration functions; circular-reference check added for moveFolder; workspace access requirements correctly upgraded to 'write'/'admin' per operation; audit logging and telemetry added for workflow creation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    UI[UI Deploy Route
/api/workflows/id/deploy] -->|POST| OD[performFullDeploy]
    UI -->|DELETE| OUD[performFullUndeploy]
    UI2[UI Deployments Route
/api/workflows/id/deployments/version] -->|PATCH| OAV[performActivateVersion]

    ADMIN[Admin Deploy Route
/api/v1/admin/.../deploy] -->|POST| OD
    ADMIN -->|DELETE| OUD
    ADMIN2[Admin Activate Route
/api/v1/admin/.../activate] -->|POST| OAV
    ADMIN3[Admin Workflow Route
/api/v1/admin/workflows/id] -->|DELETE| ODW[performDeleteWorkflow]

    CHAT_UI[Chat Route
/api/chat] -->|POST| OCD[performChatDeploy]
    CHAT_MGR[Chat Manage Route
/api/chat/manage/id] -->|DELETE| OCU[performChatUndeploy]

    COP_DEPLOY[Copilot deploy_api tool] -->|deploy| OD
    COP_DEPLOY -->|undeploy| OUD
    COP_CHAT[Copilot deploy_chat tool] -->|deploy| OCD
    COP_CHAT -->|undeploy| OCU
    COP_WF[Copilot delete_workflow tool] -->|delete| ODW
    COP_FOLDER[Copilot delete_folder tool] -->|delete| ODF[performDeleteFolder]

    FOLDER_UI[Folder Route
/api/folders/id] -->|DELETE| ODF
    WF_UI[Workflow Route
/api/workflows/id] -->|DELETE| ODW

    OCD -->|calls| OD
    OD -->|audit| AUDIT[(Audit Log)]
    OUD -->|audit| AUDIT
    OAV -->|audit| AUDIT
    ODW -->|audit| AUDIT
    ODF -->|audit| AUDIT
    OCD -->|audit| AUDIT
    OCU -->|audit| AUDIT
Loading

Comments Outside Diff (4)

  1. apps/sim/app/api/workflows/[id]/route.ts, line 2062-2063 (link)

    P2 templateAction defaults to 'orphan' even when deleteTemplatesParam is null

    The old code wrapped template handling in if (deleteTemplatesParam !== null), so templates were left completely untouched when deleteTemplatesParam was absent. The new code always computes templateAction:

    const templateAction: 'delete' | 'orphan' =
      deleteTemplatesParam === 'delete' ? 'delete' : 'orphan'

    This means any DELETE request that omits deleteTemplatesParam (e.g., direct API calls, or a frontend path that skips the template-check flow) will now silently orphan any published templates associated with the workflow, whereas previously those templates would retain their workflowId reference.

    While the archived workflow still exists in the DB (so the FK isn't broken), this is a silent behavioral change. Consider preserving the old guard:

    And handle 'preserve' in performDeleteWorkflow by skipping template modification when templateAction === 'preserve'.

  2. apps/sim/app/api/v1/admin/workflows/[id]/route.ts, line 1170-1175 (link)

    P2 Admin audit attribution uses workflow owner ID instead of an admin actor

    performDeleteWorkflow is called with userId: workflowData.userId — the workflow owner's ID. Since performDeleteWorkflow records a WORKFLOW_DELETED audit entry with actorId: userId, the audit trail will show the workflow owner deleting their own workflow rather than the admin API triggering the deletion. The same pattern occurs in the admin deploy route (performFullDeploy with userId: workflowRecord.userId).

    Previously the admin routes recorded no audit entries at all, so this is an improvement, but the actor attribution is now misleading for compliance reviewers. Consider introducing a sentinel actor identifier (e.g., 'admin-api') or a dedicated audit field for admin-sourced actions.

    This also applies to the admin deploy (/deploy/route.ts) and version-activation (/versions/[versionId]/activate/route.ts) routes.

  3. apps/sim/lib/workflows/orchestration/chat-deploy.ts, line 4519-4521 (link)

    P2 Every chat deploy now emits two audit entries: WORKFLOW_DEPLOYED + CHAT_DEPLOYED

    performChatDeploy calls performFullDeploy, which always records a WORKFLOW_DEPLOYED audit entry. Then performChatDeploy itself records a CHAT_DEPLOYED entry. Previously deployWorkflow (called from the chat POST route) was a lower-level utility that did not emit audit events, so only one CHAT_DEPLOYED event was recorded per chat creation.

    Now every chat creation/update will produce both a WORKFLOW_DEPLOYED and a CHAT_DEPLOYED entry in the audit log for the same request, which can be confusing for operators reviewing audit history. Consider either:

    • Passing an auditContext: 'chat' flag to performFullDeploy to suppress the WORKFLOW_DEPLOYED entry when called from a chat context, or
    • Documenting this intentional change in the code comment.
  4. apps/sim/lib/workflows/utils.ts, line 5447-5450 (link)

    P2 Dead exported functions: deleteWorkflowRecord and deleteFolderRecord

    After this PR removes their only callers (from mutations.ts), deleteWorkflowRecord (line ~532) and deleteFolderRecord (line ~595) in utils.ts have zero usages across the entire codebase. These are now dead code and can be removed to keep the public API surface clean and prevent accidental use that would bypass the new orchestration guards (lifecycle management, last-workflow guard, audit logging).

Reviews (1): Last reviewed commit: "improvement(platform): standardize perms..." | Re-trigger Greptile

@icecrasher321
Copy link
Copy Markdown
Collaborator Author

bugbot run

@icecrasher321
Copy link
Copy Markdown
Collaborator Author

bugbot run

@icecrasher321
Copy link
Copy Markdown
Collaborator Author

bugbot run

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

@icecrasher321 icecrasher321 merged commit 0abeac7 into staging Mar 31, 2026
11 checks passed
@icecrasher321 icecrasher321 deleted the improvement/ui-action-consolidation branch March 31, 2026 03:25
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