Skip to content

refactor(uploads): hoist execution permission check, dedupe permission-gate tests#5420

Merged
waleedlatif1 merged 1 commit into
stagingfrom
cleanup-5404-followup
Jul 4, 2026
Merged

refactor(uploads): hoist execution permission check, dedupe permission-gate tests#5420
waleedlatif1 merged 1 commit into
stagingfrom
cleanup-5404-followup

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #5404 (already merged) — applying an explicit /cleanup + /simplify final gate on that diff surfaced two small, real, low-risk quality issues that this PR fixes:

  • apps/sim/app/api/files/upload/route.ts: the new execution-context workspace permission check (getUserEntityPermissions) was re-run once per uploaded file inside the upload loop, even though workspaceId (and thus the permission result) is invariant for the whole request. Hoisted the workflowId/executionId/workspaceId validation + permission check above the loop so it runs once per request instead of once per file.
  • apps/sim/app/api/files/upload/route.test.ts: the 5 new "Execution Context Permission Gate" tests repeated identical request-construction boilerplate. Extracted a shared postExecutionUpload() helper.

No behavior change — same validation order, same error messages/status codes, same mocked call assertions. /cleanup's other 6 sub-passes (effects/memo/callbacks/state/react-query/emcn/url-state) had nothing to flag since this diff is backend-only (no React/UI code). A /simplify "reuse" suggestion to swap the permission check for a different existing helper (verifyExecutionFileAccess) was deliberately not applied — it would make the execution branch inconsistent with the 4 other context branches in the same file that use the identical getUserEntityPermissions + 403 pattern; an altitude-focused review agent independently confirmed the current pattern is the correct, consistent one for this file.

Test plan

  • bun vitest run apps/sim/app/api/files/upload/route.test.ts — 19/19 pass
  • apps/sim type-check clean (bun run type-check in apps/sim)
  • bunx biome check clean on both touched files
  • bun run check:api-validation — audit passed

…p, dedupe permission-gate tests

/simplify pass on PR #5404: resolve the execution-context workspace permission
check once per request instead of once per uploaded file, and collapse
repeated request-construction boilerplate in the new permission-gate tests
into a shared helper.
@vercel

vercel Bot commented Jul 4, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 4, 2026 10:29pm

Request Review

@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Refactor-only on upload routing and tests; authorization rules and response contracts are unchanged, with only a minor efficiency gain for multi-file execution uploads.

Overview
Moves execution-context validation and workspace permission checks in route.ts above the per-file upload loop so getUserEntityPermissions runs once per request instead of once per file; the loop now reuses a resolved executionUploadContext object.

In route.test.ts, adds a shared postExecutionUpload() helper so the execution permission-gate tests no longer duplicate FormData/request construction.

No intended behavior change — same 400/403 responses, messages, and upload paths.

Reviewed by Cursor Bugbot for commit 8c61bfa. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR is a follow-up quality pass on the execution-context upload work from #5404, making two focused cleanup changes with no behavior impact.

  • route.ts: Moved the workflowId/executionId/workspaceId validation and getUserEntityPermissions call from inside the per-file loop to a single pre-loop block, eliminating redundant permission lookups when multiple files are uploaded in one request.
  • route.test.ts: Extracted a shared postExecutionUpload() helper to deduplicate identical request-construction boilerplate across all five execution-context test cases, while keeping the same assertions and mock setup.

Confidence Score: 5/5

Safe to merge — the execution-context permission check is logically equivalent before and after the hoist, and the test refactor preserves all existing assertions.

The hoisted permission guard runs before any file is processed, which is strictly correct: workspaceId is invariant across the request, so one check is sufficient and produces the same outcome as the original per-file check. The in-loop && executionUploadContext guard is unreachable when undefined (the pre-loop block either throws or returns early), making it safe defensive narrowing. No logic paths are added, removed, or reordered in a way that changes observable behavior.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/api/files/upload/route.ts Execution-context permission check hoisted above the file loop; logic is equivalent and semantically correct — the executionUploadContext variable is always defined when the in-loop guard fires, and the && executionUploadContext narrowing is safe defensive TypeScript.
apps/sim/app/api/files/upload/route.test.ts Shared postExecutionUpload helper extracted to remove boilerplate from 5 execution-context tests; beforeEach re-ordering is harmless (it registers a callback, doesn't reference the helper), and coverage is unchanged.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[POST /api/files/upload] --> B{context === 'execution'?}
    B -- Yes --> C{workflowId / executionId / workspaceId present?}
    C -- No --> D[throw 400 InvalidRequestError]
    C -- Yes --> E[getUserEntityPermissions — once per request]
    E --> F{write or admin?}
    F -- No --> G[return 403]
    F -- Yes --> H[set executionUploadContext]
    B -- No --> I[continue to file loop]
    H --> I
    I --> J[for each file]
    J --> K{context === 'execution' AND executionUploadContext?}
    K -- Yes --> L[uploadExecutionFile]
    K -- No --> M{other context branch}
    L --> N[push result]
    M --> N
    N --> J
    J -- done --> O[return results]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[POST /api/files/upload] --> B{context === 'execution'?}
    B -- Yes --> C{workflowId / executionId / workspaceId present?}
    C -- No --> D[throw 400 InvalidRequestError]
    C -- Yes --> E[getUserEntityPermissions — once per request]
    E --> F{write or admin?}
    F -- No --> G[return 403]
    F -- Yes --> H[set executionUploadContext]
    B -- No --> I[continue to file loop]
    H --> I
    I --> J[for each file]
    J --> K{context === 'execution' AND executionUploadContext?}
    K -- Yes --> L[uploadExecutionFile]
    K -- No --> M{other context branch}
    L --> N[push result]
    M --> N
    N --> J
    J -- done --> O[return results]
Loading

Reviews (2): Last reviewed commit: "refactor(uploads): hoist execution permi..." | Re-trigger Greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 8c61bfa. Configure here.

@waleedlatif1 waleedlatif1 merged commit 0d6994d into staging Jul 4, 2026
18 checks passed
@waleedlatif1 waleedlatif1 deleted the cleanup-5404-followup branch July 4, 2026 22:40
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