feat(chat): support .zip uploads with extract into workspace files/#5564
Closed
Sg312 wants to merge 21 commits into
Closed
feat(chat): support .zip uploads with extract into workspace files/#5564Sg312 wants to merge 21 commits into
Sg312 wants to merge 21 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Sg312
changed the title
Contributor
Greptile SummaryThis PR adds zip upload support to the chat file flow. The main changes are:
Confidence Score: 4/5This is close, but the failed-extract cleanup should be fixed before merging.
apps/sim/lib/uploads/archive.ts Important Files Changed
Reviews (3): Last reviewed commit: "fix(zip-uploads): detect prior nested-on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Collaborator
Author
Collaborator
Author
Comment on lines
+374
to
+376
| for (const file of extracted) { | ||
| try { | ||
| await deleteWorkspaceFile(workspaceId, file.id) |
Contributor
There was a problem hiding this comment.
uploadWorkspaceFile has already charged storage usage for each successful upload. deleteWorkspaceFile only sets deletedAt, so a failed extraction can leave the workspace quota consumed by files the caller can no longer see or clean up. The cleanup path needs to reverse the storage/accounting side effects too, not just hide the metadata.
Sg312
force-pushed
the
dev
branch
5 times, most recently
from
July 17, 2026 18:17
477ab00 to
caa454a
Compare
…5760) * feat(mcp): reuse warm connections for tool execution and discovery * fix(mcp): make connection pool concurrency-safe and auth-scoped * fix(mcp): decouple pool validity from updatedAt, widen dead-connection detection, guard ping race * fix(mcp): retire pooled connections on auth failure and server delete * improvement(mcp): defer bulk-discovery env resolution to the pool miss path * fix(mcp): retire in-flight creates evicted mid-connect via server generation * fix(mcp): use evicted-mid-create connections one-shot and decouple pool eviction from cache clear * improvement(mcp): dedupe create thunks into buildClient, harden dispose against liveness race * fix(mcp): close acquire-gap races (re-resolve after stale ping, skip closing entries) * fix(mcp): retry auth failures in executeTool; simplify acquire re-check and clear generations on dispose * fix(mcp): let bulk discovery reconnect a lost notification connection with current config * fix(mcp): recover rotated credentials on discovery via shared fetchServerTools auth-retry * chore(mcp): add debug logging for pool hit/miss/eviction observability
#5762) Response-side Zod .catch() tolerance on the three strict-enum-over-free-text MCP columns (transport/authType/connectionStatus) so one legacy row can't fail the whole server-list validation; fork copy normalizes transport; create/upsert path resets connection status on any auth-type flip (mirrors update path); bulk discovery drops the positive tool cache on OAuth-pending. Request bodies stay strict.
… can't strand the connect (#5767) * fix(mcp): deliver OAuth callback result over BroadcastChannel so COOP can't strand the connect An MCP provider whose authorization page sets `Cross-Origin-Opener-Policy: same-origin` (e.g. Gauge) severs `window.opener` when the popup navigates through it, so the callback's `window.opener.postMessage` was silently lost and the row hung on "Connecting…" forever — even when the callback succeeded. - Signal completion over a same-origin `BroadcastChannel` instead, which is origin-scoped and immune to opener severance (the MDN/Chrome-recommended COOP workaround). Scope the message by workspaceId so other open workspaces ignore it. - Log every OAuth callback failure with its reason + serverId. The early-return gates previously returned a silent `ok:false` popup close, so a failed authorization was undiagnosable from the server logs. * fix(mcp): react to the OAuth broadcast only in the tab that opened the popup A BroadcastChannel reaches every same-origin tab, so the previous workspace-id scoping (which the success path carried but failure paths omitted) still let unrelated tabs clear state, refetch, and show spurious toasts. Gate every reaction on whether this tab actually has an in-flight popup for the result's server (`popupIntervalsRef`) — strictly more precise than workspace scoping and correct for both cross-workspace and same-workspace-second-tab cases. Removes the now-redundant workspaceId from the callback message. * fix(mcp): decouple OAuth result correlation from popup.closed Cursor flagged two defects in the previous gate, both rooted in keying the BroadcastChannel filter on `popupIntervalsRef` (the popup.closed poll map): - A genuine completion was dropped whenever the poll had already removed the server's entry — and COOP can make `popup.closed` misreport, which is exactly the case this PR targets, so the fix could silently fail to apply. - A result without a serverId fell back to "any in-flight popup", waking unrelated same-origin tabs with spurious toasts/refetches. Introduce `pendingFlowsRef` (serverId -> safety timeout) as the correlation source of truth: cleared only on completion or a 10-min timeout (matching the server OAuth start TTL), never by popup.closed. The popup.closed poll now only clears the spinner (best-effort abandon UX) and never touches correlation, so a real completion is always processed. Results without a serverId are ignored; the initiating tab's safety timeout clears its own "Connecting…". * fix(mcp): correlate the OAuth result on the state nonce, not serverId Cursor flagged that a failure which can't resolve a serverId (notably invalid_state) broadcasts ok:false with no serverId, so the serverId-keyed gate ignored it and the initiating tab sat on "Connecting…" until the safety timeout with no error feedback. Correlate on the OAuth `state` instead — the per-flow nonce the callback echoes on every result, success or failure. The client parses it from the authorization URL and keys the in-flight map by it; the callback includes it on every response via a `respond` helper. This is the canonical popup-OAuth correlation: it reaches the initiating tab even when no serverId exists, and — because each flow has a unique state — it also fixes the same-user-same-server-in-two-tabs edge a serverId key left open. Results with no parseable state (a malformed callback) are still ignored; that flow clears via its timeout. * fix(mcp): drop a server's prior in-flight OAuth flow when it is retried Each start mints a new `state`, so an abandoned attempt's safety timeout was keyed under a different state than its retry and never cleared. In the contrived case where both stayed pending ~10 min, the stale timer would clear the newer flow's spinner. Sweep any prior in-flight flow for the same serverId on a new start (replacing the can't-happen same-state check). Result delivery was already correct; this makes the state machine airtight.
* Simplify TikTok to draft-only Content Posting Remove Direct Post stubs and legacy Share Kit webhook triggers that Sim does not ship, and fix draft upload response handling so real TikTok errors are not misreported as size-limit failures. Co-authored-by: Cursor <cursoragent@cursor.com> * fix TikTok cleanup lint and Greptile review findings Reject legacy mode:direct publish requests instead of silently drafting, keep deprecated Share Kit triggers registered for saved workflows, and apply biome format/import fixes. Co-authored-by: Cursor <cursoragent@cursor.com> * Finish TikTok greenfield draft-only surface Remove Query Creator Info and video.publish, drop Share Kit trigger stubs, rename publish-video to upload-video-draft, and strip leftover Direct Post mode from the contract. Co-authored-by: Cursor <cursoragent@cursor.com> * Hide TikTok from the toolbar until app review is approved. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Cursor <cursoragent@cursor.com>
…te to the agent (#5656) Implement the sim-side share_file server tool (resolves VFS path to file, keeps the existing org-policy + permission + audit checks, delegates to upsertFileShare). Register it in the tool router and generated catalog. Stamp an ambient 'shared'/'shareAuthType' flag onto file metadata via one batched share lookup, mirroring how workflows expose 'isDeployed'. Validate the EFFECTIVE auth type when re-enabling a share: upsertFileShare preserves an existing share's authType when none is passed, so validate the stored mode (not 'public') or a re-share could reactivate a now-disallowed password/email/sso share. Same fix applied to the share PUT route.
…clobber (#5772) The MCP OAuth callback fails with invalid_state because the authorization state is cleared/missing by the time the user authorizes — but clearState was silent, so the clobber never surfaced in logs. Log every state save and clear with a caller context so the exact source is visible on the next repro.
…rand over first/last rows (#5774)
…; document DCR match (#5777)
…emplate title (#5775) * improvement(blocks): name the integration in every suggested-action template title Home-screen "Suggested actions" rows render only an icon + the block template title, so titles that did not name their integration (e.g. "Refund pattern monitor") were unidentifiable. Rewrite the 234 titles that omitted their integration so each names it naturally, matching each block's existing "Brand + phrase" style. Titles-only change; the catalog surfaces that already show the integration as page context are unaffected. * fix(blocks): name PagerDuty-triggered incident template after its icon, not owner The Confluence-owned incident template uses a PagerDutyIcon, is triggered by PagerDuty, and is cross-listed to notion/pagerduty/datadog/slack, so prefixing the owner name misidentified it on those surfaces and fought its own icon. Name it after the icon + trigger integration instead. * fix(blocks): align suggested-action icons with titles and de-dupe Firecrawl names For rows where the owner-prefixed title named a different integration than the row's icon (github/Notion, stripe/Sheets, calendar/Twilio, gmail/Lemlist, slack/Linear, linear/Slack), swap the icon to the owner integration the title already names so the icon-only home row is coherent; drop the now-unused icon imports. Differentiate two Firecrawl titles that collided with existing near-duplicate templates. * fix(blocks): de-duplicate suggested-action titles the brand prefix collided Renamed a handful of titles whose brand prefix made them near-identical to a sibling template: google_translate (Intercom replier / ticket auto-reply), sentry (on-call triage agent), google_drive (personal notes assistant), sqs (alert enricher), typeform (survey summarizer). Swept every renamed block for the same near-duplicate class. * fix(blocks): align Greptile Slack Q&A bot icon with its title The renamed title leads with Greptile but the row kept icon: SlackIcon, conflicting on the icon-only home surface. Swap to GreptileIcon (matching the block's sibling templates) and drop the orphaned SlackIcon import.
* fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout The MCP SDK issues OAuth discovery, dynamic client registration, and token exchange with a bare fetch and no AbortSignal (only the JSON-RPC layer gets the SDK's request timeout), and undici's default headers/body timeouts are 5 min. A slow or unresponsive authorization server therefore left /oauth/start pending for minutes — the browser stuck on "Connecting…" forever. Bound each guarded OAuth/revocation leg with a 30s AbortSignal.timeout, composed with any caller signal so cancellation still works. Only our own deadline is relabeled to an McpError; caller aborts and other failures propagate unchanged. Scoped to createSsrfGuardedMcpFetch (OAuth/revoke/probe) — the live transport's timeouts are untouched. * fix(mcp): bound SSRF/DNS validation by the deadline too Move the timeout signal ahead of validateMcpServerSsrf and race the validation (whose dns.lookup takes no signal) against it, so the deadline covers the whole guarded call — a stalled DNS resolution now rejects at timeoutMs instead of the OS DNS timeout. Listener is cleaned up on settle so a late timeout can't surface as an unhandled rejection. * fix(mcp): compose caller signal before validation + attribute timeout by reason Compose the caller's AbortSignal with the deadline up front and use the combined signal for both SSRF validation and the HTTP request, so caller cancellation now covers the whole guarded call (previously a caller abort during a stalled DNS validation waited for the full deadline). Attribute the timeout relabel by the rejection reason's identity rather than init.signal's state, so a caller signal that aborts just after the deadline can't misattribute a genuine timeout. * fix(mcp): adopt in-flight validation on early abort to avoid unhandled rejection When raceWithSignal is entered with an already-aborted signal it returned without attaching to the in-flight validateMcpServerSsrf promise, so a later SSRF/DNS rejection could surface as an unhandled rejection. Swallow the orphaned promise's settlement in the early-abort branch. * test(mcp): use sleep() instead of raw setTimeout in pinned-fetch test check:utils bans new Promise(resolve => setTimeout(...)) in favor of sleep() from @sim/utils/helpers.
* feat(pi): add Cloud Code Review mode and rename Cloud to Cloud PR Introduce a third Pi mode that reviews an existing GitHub PR in an E2B sandbox and posts a structured review with optional inline comments. Keep the stored cloud id for backward compatibility, extend github_create_pr_review for inline comments, and harden review submission against stale SHAs and invalid comment payloads. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(pi): cleanup code * address comments * address mor * address comments * update --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
…ds (#5783) * fix(library): show full unambiguous dates on featured and related cards * fix(library): format content dates in UTC to avoid off-by-one day
#5781) * improvement(ux): submit on Enter in chip modals and advance table rows * fix(emcn): stop Enter double-submit and close registration gap - stopPropagation on field Enter-submit so a parent onKeyDown can't re-fire the primary (double OAuth connect, etc.) - register primary via useLayoutEffect so it's set before paint (no null-Enter window) - drop now-redundant parent Enter handlers in connect-oauth/create-workspace/rename-document modals * fix(table): suppress auto-opened tag dropdown when Enter advances cells Focusing an empty cell auto-opens the tag dropdown; without closing it a follow-up Enter inserts a tag instead of navigating down the column. * fix(table): clean up cell refs on unmount and guard Enter advance - delete inputRefs/overlayRefs entries when a cell detaches so deleting a row can't leave stale position-keyed entries - guard Enter advance with isConnected so a stale ref can never steal focus into a detached node
…nsport (#5782) * diag(mcp): comprehensive HTTP logging for OAuth + streamable-HTTP transport Temporary diagnostic to root-cause the Gauge MCP 'initialize' hang. Wraps both the OAuth guarded fetch and the transport pinned fetch to log every request/response with timing: method, url, status, content-type, mcp-session-id, safe headers, and — for the transport phase only — the response body streamed chunk-by-chunk. So one authorize reveals exactly what Gauge returns for initialize (SSE that stalls vs never-sent result vs fast JSON) and where it stalls. Enabled by default; MCP_HTTP_DIAGNOSTICS=false to silence; disabled under test. Never logs request bodies or the OAuth token-response body (carry tokens); every logged value passes through sanitizeForLogging. Revert once root-caused. * diag(mcp): scope body logging to initialize + redact URLs + wrap unpinned Review fixes (Greptile/Cursor): - Only stream-log the response body whose REQUEST is an MCP initialize JSON-RPC call. The transport fetch also carries in-transport OAuth refresh and every tools/call result, so gating on phase alone leaked token responses and tool output (PII/file contents). initialize gating excludes both. - Redact URL query strings (?code=/?token=/?api_key=) — log origin+path only. - Wrap the transport fetch whether pinned or not (SDK default is globalThis.fetch, so passing it is behavior-neutral) so unpinned/allowlisted servers are covered too. * diag(mcp): sanitize initialize preview + log at warn for visibility Review fixes (Cursor): - Run the initialize body preview through sanitizeForLogging (defense-in-depth against a server stuffing token-like values into serverInfo/capabilities). - Log diagnostics at warn, not info, so they surface even if an environment's LOG_LEVEL drops info (staging runs INFO, but this removes the dependency). * diag(mcp): reuse sanitizeUrlForLog + cancel log reader on abort Review fixes (Cursor): - Replace the local safeUrl helper with the shared sanitizeUrlForLog so URL redaction/truncation stays consistent. - The detached initialize-body log reader now observes init.signal and cancels its tee-branch reader on abort, so it can't keep the response stream / connection alive after the SDK's initialize timeout gives up (the hang we're tracing). * diag(mcp): length-gate initialize check to skip parsing large tool payloads isInitializeRequest now rejects bodies over 4096 chars before any JSON.parse. The initialize message is a small fixed structure, so this keeps large tools/call payloads (potentially multi-MB) off the parse hot path while still detecting initialize.
…5785) * feat(blocks): surface deprecated block and model warnings on canvas * improvement(blocks): simplify deprecation derivation, drop unused getModelReplacement * fix(blocks): make deprecation badge keyboard-accessible
- Replace the whole-buffer central-directory signature scan with an EOCD-anchored walk (shared with zip-guard) so zips containing STORED nested archives are no longer falsely rejected, and report the accurate cap (new 'central_dir_too_large' reason) instead of 'Maximum is 1000' - Make extraction all-or-nothing: a discarding validation pass inflates every entry against the caps before anything is uploaded, so lying headers or corrupt entries can't leave partial trees (corrupt DEFLATE streams now surface as ArchiveError instead of raw zlib errors) - Single-source ArchiveError messages; callers surface err.message and map only status/reason (removes the three divergent message maps) - Guard save/import against archives (saving stranded the contents), extend the workspace ownership check to all three operations, dedupe fileNames, and refuse re-extracting into a non-empty folder instead of duplicating the tree with ' (1)' copies - Harden the extraction folder name (dot segments, control chars, separators) and compute the destination before extracting - Sniff small '.zip'-named uploads so mislabeled text files stay readable instead of dead-ending between read and extract - Scope zip acceptance to the mothership flow only (attachment list, accept attribute, upload/presigned gates) so execution/workspace/chat surfaces keep rejecting zips up front - Don't count skipped noise entries toward the 1000-file cap; restore per-entry zip-slip forensics via skippedUnsafePaths logging - Teach materialize_file(fileNames: [...]) in the extract guidance to match the declared schema
- Roll back already-uploaded files when an upload fails mid-extraction (storage/DB error, quota crossed), so callers and retries never observe a partial tree - Fold reserved system folder names (.changelogs, .plans) into the 'archive' fallback so extraction can't write into alias-backing namespaces or bypass the already-extracted lookup that hides them - Align the archive byte-sniff budget with the read path's inline text cap (5MB), so any mislabeled '.zip' small enough to be read inline is sniffed and read instead of dead-ending
…act guard The already-extracted check only looked for files directly inside the archive root folder, so a zip whose entries are all nested (src/index.ts) left only subfolders there and a second extract slipped past the guard, duplicating the tree with ' (1)' suffixes. Extraction roots its whole tree at that folder, so a prior run always leaves a direct file OR a direct subfolder — check both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
.zipsupport to copilot chat uploads with an explicit extract-first flow: a zip is stored once in the chat'suploads/namespace, and the agent unpacks it withmaterialize_file(operation: "extract")into afiles/<archive>/folder tree, where the contents are readable with the normalfiles/tooling. Reads/greps of a raw zip return extract-first guidance instead of binary bytes. The shared extraction primitives (zip-bomb / zip-slip / symlink guards, size caps) are factored out of the file-manage decompress route intolib/uploads/archive.tsand reused by both paths.The branch also hardens the extract path from a deep review pass: the zip-bomb pre-scan now walks the real EOCD-anchored central directory (reusing
file-parsers/zip-guard) so zips containing stored nested archives are no longer falsely rejected; extraction is all-or-nothing (a discarding validation pass proves the caps before anything is uploaded, so lying headers or corrupt entries can't leave partial trees);save/importrefuse archives (saving stranded the contents unreachably); re-extracting into a non-empty folder is refused instead of duplicating every file asa (1).txt; degenerate names (..zip, control chars) fold into a safe fallback folder; small mislabeled.zipfiles are byte-sniffed so they stay readable; and zip acceptance is scoped to the mothership flow only (execution/workspace/deployed-chat upload gates keep rejecting zips up front).ArchiveErrormessages are single-sourced with accurate caps, and per-entry zip-slip forensics are logged again.Type of Change
Testing
archive(incl. new regression tests: nested stored-zip false positive, corrupt-entry all-or-nothing, noise entries not counting toward the 1000-file cap, synthetic central-directory cap tests),materialize-file(save-on-zip guard, already-extracted guard, fileNames dedupe, degenerate-name fallback, cross-workspace guard),upload-file-reader(zip guidance, large-zip no-download, mislabeled-zip readable),zip-guard,validation,file-utils,payload, and the upload/presigned routestsc,biome, andcheck:api-validationall cleantool-catalog-v1.ts/tool-schemas-v1.tsregenerated from the companion's contract and byte-verified in synclib/uploads/archive.ts, the gate scoping inlib/uploads/utils/validation.ts+ upload/presigned routes, and the extract guards inlib/copilot/tools/handlers/materialize-file.tsfiles/(compress-op output, nested zips), and the per-entry sequential upload loop / whole-archive buffering inherited from the old decompress routeChecklist
Companion PR
Companion: https://github.com/simstudioai/mothership/pull/348