diff --git a/ISSUES.md b/ISSUES.md new file mode 100644 index 00000000000..d33f1eb5c78 --- /dev/null +++ b/ISSUES.md @@ -0,0 +1,303 @@ +# Open issues — chat-scoped outputs / fork PR (#5401) + +## Release triage decision (2026-07-05) + +All 11 findings below ship as **tracked fast-follows** — none are fixed on +this branch. The feature releases behind the mothership flag via staged +rollout. Conditions of this acceptance: + +- The two review-round fixes that landed without tests are now pinned + (`tool-call-state.test.ts`, `contracts/copilot.test.ts`). The hook-internal + half of the reconnect-wedge fix (`streamStarted` gating, terminal 404) has + no testable seam in `use-chat.ts`; extracting one is part of the issue 9 + fast-follow. +- Issues 1 (quota ratchet) and 3 (fork crash window) need monitoring during + rollout: alert on fork `failedFileCopies` counts and watch storage-quota + support signals. +- Issues 6, 7, 8, 11 still need the product calls noted inline before their + fast-follows can be scoped. + +## Max-effort review pass (2026-07-05): fixed + fast-follows + +A 10-angle adversarially-verified review of the frozen branch surfaced 35 +findings (all file:line refs verified). Root cause of the worst cluster: the +by-id resolvers pin `context='workspace'`, so `output` rows were invisible to +every path not explicitly rewired for the third namespace. + +**Fixed on this branch** (each with a regression test where a seam exists): + +- Model payload drops output tabs — `resolveFileResource` now falls back to + `resolveChatFileRecordById` and emits paths via the shared + `chatScopedOrWorkspacePath` (moved to `vfs/path-utils`). +- Output tabs offered a working editor whose every save failed — + chat-scoped records now render read-only (`resource-content.tsx`). +- CSV outputs >5MB 404'd in CsvTablePreview — the csv-preview route resolves + via `getPreviewableWorkspaceFile` (owner-gated) and serves from the row's + real storage context. +- Presign-flow UUID upload ids never resolved as tool inputs — the chat + by-id fallback in `resolveToolInputFile` dropped its `wf_` prefix gate. + +A client-side resume-probe fix for the send-path double-send was attempted +and REVERTED after the gate re-review refuted its premise: run registration +happens at response-construction time (`createRunSegment` inside +`ReadableStream.start`, after the multi-second server prep), so a probe 404s +terminally during exactly the ambiguous window it targets — and the await it +inserted before the rollback opened a new lost-message window on Stop. See R0. + +**New fast-follows — correctness/silent-failure (fix in priority order):** + +- R0 `use-chat.ts:3436` + `lib/copilot/chat/post.ts` — double-send: a POST + that dies without a response is blindly rolled back; the queued dispatch + restores and re-sends a message the server may already be answering. + Correct fix is server-side: make the send idempotent by `userMessageId` + (dedupe in `persistUserMessage` / extend the pending-stream 409 to cover + the post-completion window) or register the run BEFORE prep so a client + probe can see it. A client probe is only sound after that lands, built on + an extracted, unit-testable send/reconnect state machine (pairs with + item 9's fast-follow). Verified nuance (2026-07-05 audit): the user message + row itself does NOT duplicate — `appendCopilotChatMessages` upserts on + `(chatId, messageId)` — the duplication is a second full assistant run; + that upsert is a natural anchor for the idempotency check. +- R1 `workspace-file-manager.ts:500` — trackChatUpload's claim UPDATE has no + "unclaimed" guard: any re-track of the same key moves `message_id` (fork-cut + birthdate) to the later message; callers omitting messageId NULL it. +- R2 `chat-file-reader.ts:203` + `workspace-file-manager.ts:1097` + + `hooks/queries/workspace-files.ts:133` — the swallow-to-success family: + listChatFiles returns [] on DB error (outputs route 200s), previewable + lookup returns null on DB error (route 404s), useWorkspaceFileById caches + null on any error incl. AbortError. Fix as one convention: throw on + lookup failure, reserve null for not-found/denied. +- R3 `create-file.ts:57` + `workspace-file.ts:359` — create guards reject only + `outputs/`, not `uploads/` (delete/rename/folders cover both) — an + `uploads/…` create mints a literal files/uploads/ folder + self-confusion. +- R4 `use-prompt-editor.ts:282` — @-mention completion misses chat outputs + (`useAvailableResources` called without chatId; needs threading to + UserInput). Same file offered by the "+" picker. +- R5 `hooks/queries/mothership-chats.ts:684` — forkChat truthiness collapses + `upToMessageId: ''` into whole-chat duplicate (quota-charged) instead of + surfacing the contract 400. Latent; both callers currently guard. +- R6 `lib/uploads/server/metadata.ts:168` — empty context array drops the + context filter (matches ANY context) on an authz-feeding helper; make [] + match nothing. Latent; sits on the access path. +- R7 `doc-compile.ts:150` (inventory finding) — referenced images resolve + workspace-only; an agent-generated output referenced in a doc silently + drops. Verified (2026-07-05 audit): `CompileArgs` carries no chatId, so the + fix needs a signature change — thread `context.chatId` from the callers + (e.g. `edit-content.ts` already has it) into `compileDoc`, then route + through resolveToolInputFile. +- R8 (manual testing 2026-07-06, plan test 2) — flag-on `create_file` schema + contradicts the sim guard: mothership advertises `outputs/` as a + valid destination (`catalog/files/canonical_resources.go:10`, wired in + `catalog/files/create_file.go:23`) while sim's server tool unconditionally + rejects outputs/ targets (`create-file.ts:57`) with advice to "create + editable files under files/ instead" — so an explicit "create + outputs/notes.md" reliably lands at `files/notes.md` (leaf only), with the + agent confidently narrating the fallback. Deterministic, not model flake. + Deeper gap: NO tool is prompt-encouraged to create a *text* output. The + only context-threaded path is function_execute's returned-value + `outputs.files` write (`request/tools/files.ts:260`), but function_execute's + own description forbids ordinary file creation and points back to the + dedicated file tools — circular steering. Fix: (a) create_file must use the + files-only path description regardless of flag (the shared + `canonicalFileOutputsParameter` stays correct for + download_to_workspace_file, where outputs/ IS valid); (b) product/prompt + call on how an explicit text-output request should be fulfilled — bless + function_execute `outputs.files` for text one-offs, or declare text outputs + generator-only and fix MANUAL-TESTING test 2's expectation. Note the sim + guard itself is coherent: create_file allocates an EMPTY file and outputs + are write-once, so create-then-edit can never produce a text output. + Companion-repo touch: mothership `catalog/files/create_file.go`. + +**New fast-follows — cleanup/altitude batch (one PR, mostly mechanical):** + +- Twin display-name allocators: `uploadChatOutput` vs `trackChatUpload` + (`workspace-file-manager.ts:598`) — unify like the read side. +- Output-ownership rule at three sites (`authorization.ts:271/708`, + `workspace-file-manager.ts:1087`) — single chokepoint accessor. +- Per-file `incrementStorageUsage` in fork blob copies (`fork-chat-files.ts:205`) + — accumulate, charge once per fork. +- Eager flag-independent `useChatOutputs` fetch on every chat mount + (`home.tsx:261`); serialized by-id fallback in `resource-content.tsx:536`. +- Sequential independent awaits: fork route messages+files (`route.ts:121`), + reference-image/ffmpeg download loops (`generate-image.ts:86`, `ffmpeg.ts:83`). +- Duplicated fork/duplicate title formula client+server + (`mothership-chats.ts:707` vs fork `route.ts`); duplicated failedFileCopies + toast (`message-actions.tsx:159` / `sidebar.tsx:980`). +- `isPersistedChatResource` vs shared `isEphemeralResource` (`use-chat.ts:160`); + vfs grep's own namespace regexes vs canonical predicates (`vfs.ts:50`); + three-way namespace routing copy-pasted in materialize save/import + (`materialize-file.ts:137`); context trio threading in resource-writer + (`resource-writer.ts:305`); fork quota gate re-deriving copyability + (`route.ts:129`); outputs route hand-rolled 404/500 + manual request-id + logging in files by-id route (`outputs/route.ts:38`, `files/[fileId]/route.ts:33`); + decode-fallback inlined 3× (`chat-file-reader.ts:66/120`, `resource-writer.ts:335`); + ten per-namespace one-line wrappers (`chat-file-reader.ts:257`); fork-route + skip-update guard re-deriving rewrite internals (`route.ts:195`); mothership + resource contracts parity-pinned by test instead of shared schema objects + (`mothership-chats.ts:174`). + +Working list of verified findings **not yet fixed on this branch**. These are +candidates to resolve before merge or explicitly sign off on — being listed +here is not acceptance. Each entry links the mechanism, the verified impact, +and the known fix design. Findings already fixed on the branch are not listed. + +## 1. Quota is never refunded for chat-scoped bytes (charged by this PR) + +- **What:** This PR is the first to charge storage quota for chat-scoped bytes + (`uploadChatOutput` and fork blob copies both call `incrementStorageUsage`). + No deletion path in the platform decrements the counter (the only + `decrementStorageUsage` caller is the content-shrink branch of + `updateWorkspaceFileContent`), so chat deletion / retention cleanup frees + the bytes but the counter ratchets up forever. Repeated duplicate+delete + cycles on a media-heavy chat can walk a user to their plan cap with zero + net bytes stored, permanently blocking all uploads/outputs/forks + (`checkStorageQuota` reads the raw counter). +- **Pre-existing context (answering "is there an existing bug here?"):** + (a) chat uploads were never charged — the presigned mothership path has no + increment, and multipart *checks* quota but never increments (existing + inconsistency); (b) workspace files ARE charged and their deletion/purge + never refunds either — the never-refund convention is platform-wide and + pre-existing. This PR extends it to a class of bytes that is invisible in + the Files UI, charged by agent actions, and freed implicitly. +- **Fix design (follow-up migration PR):** add a `quota_charged` marker column + stamped where the increment happens (fork-copied `mothership` rows are + otherwise indistinguishable from never-charged originals); refund on both + cleanup paths keyed to blob-delete success (retry-safe); consider an admin + reconciliation job (recompute counter from live charged rows). Wrinkles + documented by verification: collectChatFiles must start selecting + size/userId/workspaceId; org-vs-personal counter scope is resolved at call + time so late refunds can land on a different counter (pre-existing hazard). + +## 2. Interactive chat DELETE orphans blobs (pre-existing, amplified) + +- **What:** Both interactive delete routes (`DELETE /api/mothership/chats/[chatId]`, + `/api/copilot/chat/delete`) hard-delete the chat row; the `workspace_files` + FK cascade destroys the rows before any blob cleanup could read the keys — + blobs orphan in S3 forever. Only the background retention path does + collect-then-delete (`prepareChatCleanup`). Pre-PR this leaked only chat + uploads; outputs and fork copies (potentially GBs per duplicate) now leak too. +- **Fix:** make both routes collect blob keys first, delete the chat, then + best-effort delete the blobs — the exact pattern `prepareChatCleanup` + already implements. Pairs naturally with issue 1 (same cleanup path). + +## 3. Fork crash window leaves committed rows with no blobs + +- **What:** The fork transaction commits copy rows, then blobs copy in-process + post-commit. A crash/deploy restart in that window (minutes for media-heavy + chats) leaves rows whose reads/serves 404 forever; nothing reconciles + row-without-blob, and a retried fork mints fresh rows/keys rather than + healing the old ones. +- **Fix:** move blob copies to a persisted background job (trigger.dev), like + the workspace fork's `fork-content-copy` task — which also resolves issue 4. + Interim: a reconciliation sweep or accept with monitoring. + +## 4. Fork blob copies have no cross-request memory bound + +- **What:** Each fork buffers up to 4 × 100MB concurrently (download buffer + held through upload); the pool is per-request with no global limiter, so + N concurrent media-heavy forks can pin N × ~400MB+ on the web process + (PLAUSIBLE OOM under load). +- **Fix:** module-level semaphore bounding total concurrent blob-copy work + (~10 lines), or the background job from issue 3, or streaming/provider-side + copy in storage-service. + +## 5. Chat files over 100MB can never survive a fork/duplicate + +- **What:** Multipart chat uploads are allowed up to 5GB, but fork blob copies + download with `maxBytes: MAX_FILE_SIZE` (100MB) — deterministic failure, + copy row hard-deleted, transcript embeds 404, only a `failedFileCopies` + count as signal. +- **Fix options:** surface WHICH files were skipped (cheap, honest); streaming + copy (proper, pairs with issues 3/4); or cap chat upload size to match. + +## 6. Chat-scoped glob ignores the pattern past the namespace prefix + +- **What:** `glob("outputs/*.png")` returns every output (and the identical + pre-existing flaw exists for `uploads/`), while the workspace half of the + same result honors patterns via micromatch — the model gets wrong results + for specific patterns. +- **Status:** needs clarification with engineering — the uploads behavior is + pre-existing and the outputs branch deliberately mirrored it for symmetry. + Fix is one `micromatch.isMatch` filter per branch if we decide patterns + should be honored (both namespaces should change together). + +## 7. Sandbox-export writes can't produce chat-scoped outputs + +- **What:** `function_execute`'s sandbox file exports (route-side + `maybeExportSandboxFilesToWorkspace`) never receive chat context, so an + interactive `outputs/` export silently lands in workspace `files/` + (single-file form) or 400s (multi-file form) while the agent references an + outputs/ path that doesn't resolve. The in-process tool-result writer was + fixed on this branch; the route half remains. +- **Fix design:** thread `chatId`/`interactive`/`messageId` through the + function-execute contract's `_context`, and — because this is a lower-trust + boundary where the caller supplies `_context` — validate chat ownership + (`copilot_chats.user_id === auth.userId`) before honoring the chat context + for an outputs/ write. +- **Confirmed mechanism (contract audit):** `functionExecuteContract`'s body + declares no `_context.chatId`, and the tool body builder + (`tools/function/execute.ts` ~156) forwards only a whitelist of `_context` + keys — so the `chatId` the copilot handler stamps into `_context` never + reaches the route today. The fix must touch the contract, the whitelist, + and the route. + +## 8. Chat namespaces shadow workspace folders named "uploads"/"outputs" + +- **What:** With a chatId present, `uploads/…`/`outputs/…` spellings resolve + only against the chat namespaces, so a real workspace folder literally + named `uploads` or `outputs` is no longer addressable by its bare spelling + in resolver-switched tools (the canonical `files/uploads/…` spelling still + works everywhere, and glob/read hints emit canonical paths). The no-chatId + case was fixed on this branch (falls through to the workspace resolver). +- **Status:** accept + document, or reserve those two folder names at + creation time. Needs a product call. + +## 9. home.tsx silently drops unresolved file-resource clicks + +- **What:** Clicking an agent-emitted file link before the workspace-files or + chat-outputs query settles logs a warning and does nothing (pre-PR the tab + opened and resolved lazily). Window is narrow (an invalidation on file + generation was added) but nonzero; recovery is a second click. home.tsx also + carries its own copy of the outputs-by-leaf-name matcher that differs subtly + from resource-content's (raw-encoded leaf matching). +- **Fix:** defer/retry resolution instead of dropping; extract one shared + leaf-match helper used by both call sites. + +## 10. Fork purge can delete another chat's blobs on consolidated-bucket deployments + +- **What (mostly pre-existing, narrowed by this PR):** chat-cleanup's second + key source scoops `fileAttachments[].key` from all messages with no + ownership check and deletes under a hardcoded `copilot` context. A fork + whose messages carry unmapped source-chat keys (only soft-deleted or + workspaceId-less rows since this PR's rewrite; ALL attachments pre-PR) can, + when purged, delete the source chat's blob — but only when the copilot + bucket env vars point at the same physical bucket as the workspace bucket + (non-default, documented-separate deployment shape). +- **Fix:** scope the attachment-key sweep to keys owned by the chat's own + rows, or drop the hardcoded context in favor of per-row context. + +## 11. File-folder resources offered in the picker but rejected on add (pre-existing) + +- **What:** the add-resource dropdown offers `filefolder` entries, but the + copilot POST handler's `VALID_RESOURCE_TYPES` allow-list omits + `filefolder`/`task`/`generic`, so clicking one 400s ("Invalid resource + type") and the optimistic tab rolls back. Pre-existing — before the + contract sync it 400'd at the schema layer instead; same outcome. (The + sync did fix `integration` adds, which the route allowed but the schema + rejected.) +- **Fix options:** add `filefolder` to the route allow-list if folders are + meant to be addable resources (the UI clearly thinks so), or stop offering + them in the picker. `task`/`generic` look system-managed — likely correct + to keep un-addable, but confirm. + +## Accepted on this branch (for the record) + +- **Flag-off writer interception:** with the mothership flag off, a + hallucinated `outputs/` write path creates a chat-scoped output instead of + erroring (pre-PR behavior). Accepted: the file persists, is downloadable, + and appears in that chat's resource picker (flag-independent UI); truly + gating it would require plumbing the mothership flag into sim. The + write-once error now names the `files/outputs/…` spelling so a model that + meant a real workspace folder recovers in one turn. +- **Reference-image strictness and fork quota-charging** are deliberate, + documented decisions of this PR (see PR body), re-confirmed during review. diff --git a/MANUAL-TESTING.md b/MANUAL-TESTING.md new file mode 100644 index 00000000000..912ad934dfe --- /dev/null +++ b/MANUAL-TESTING.md @@ -0,0 +1,213 @@ +# Manual test plan — chat-scoped outputs + Fork/Duplicate (PR #5401) + +Ordered from smoke tests to edge cases. Each test lists exact steps, the +expected result, and what a failure would implicate. Tests marked +**[KNOWN — observe, don't file]** exercise a tracked ISSUES.md item: confirm +the behavior matches the entry, but a "failure" there is expected. + +## Setup + +- **S1.** Run sim + mothership dev servers, migrations applied through `0255` + (if the DB predates the branch, run the `0255` pre-check from the PR body: + no duplicate `(chat_id, display_name)` rows `WHERE context='output'`). +- **S2.** Enable the mothership `chat-scoped-outputs` flag. All parts assume + flag ON except Part E's flag-off test. +- **S3.** Test assets on disk: any small image (`test.png`), a 5-row + `small.csv`, and a >5MB CSV: + `python3 -c "print('a,b,c'); [print(f'{i},{i*2},xxxxxxxxxxxxxxxxxxxxxxxx') for i in range(250000)]" > big.csv` +- **S4.** Keep the browser devtools console open — several fixed bugs used + to manifest only as `logger.warn` lines. +- **S5.** Optional but valuable: a second user account that is a member of + the same workspace (for test 36). + +## Part A — Outputs namespace basics + +- **1. Generate an output.** New chat → type: *"Generate an image of a red + circle on a white background."* + **Expect:** tool call succeeds; the reply embeds/links the image; the "+" + resource picker's Files group lists it (e.g. `outputs/…png`); it does NOT + appear on the Files page. +- **2. Create a text output. [KNOWN — R8, observe don't file]** Same chat → + *"Create a file outputs/notes.md with the content 'hello world'."* + **Expect (current, tracked):** the naive phrasing FAILS deterministically — + `create_file`'s flag-on schema advertises `outputs/` but the sim tool + rejects it, and the agent follows the error's advice into a stray + `files/notes.md` (leaf only — not `files/outputs/notes.md`). That is R8. + If it happens: DELETE the stray `files/notes.md` (it will otherwise skew + tests 7 and 25's naming), then create a real text output with the explicit + workaround: *"Run code that returns 'hello world' and save the result via + function_execute outputs.files to outputs/notes.md."* That path threads + chat context and produces a genuine chat-scoped output. + **Expect (after workaround):** agent confirms; `outputs/notes.md` appears + in the "+" picker; it does NOT appear on the Files page. Tests 3–8 and 29 + assume this file exists. +- **3. Open as a tab — and confirm read-only (round-6 fix).** Click the + `outputs/notes.md` link in the reply (or add via "+"). + **Expect:** tab opens, content renders, and there is NO editor — no + editable text area, no autosave, no repeating save-error toasts. The image + output from test 1 previews normally in its own tab. +- **4. Active-tab context (round-6 fix).** With the `outputs/notes.md` tab + active, send: *"Summarize the file in my active tab."* + **Expect:** the model answers about notes.md specifically. Failure mode + this guards: model asks "which file?" or answers about something else. +- **5. Write-once enforcement.** Send: *"Overwrite outputs/notes.md with + 'goodbye'."* + **Expect:** the tool FAILS with an error that mentions outputs are + write-once and names the `files/outputs/…` spelling. No silent replace. +- **6. Same-name collision.** Send: *"Create outputs/notes.md again with + different content."* + **Expect:** a suffixed sibling (e.g. `notes (1).md`) — never an overwrite, + never a DB error surfaced to chat. +- **7. Materialize to workspace.** Send: *"Save outputs/notes.md to my + workspace files."* + **Expect:** file appears on the Files page under `files/`; the chat output + still exists in the picker. +- **8. Tab actions.** On an output tab: Download works; the "Open in files" + button is HIDDEN (outputs dead-end on the Files page by design). +- **9. Large CSV output preview (round-6 fix, best-effort).** If you can + produce a >5MB CSV *output* (e.g. via a table export to outputs if + available), open it as a tab. + **Expect:** the table preview loads rows — pre-fix this was a permanent + "File not found". If you can't produce one, skip — the route is pinned by + `csv-preview/route.test.ts`. + +## Part B — Fork & Duplicate + +- **10. Basic duplicate.** Build a chat: 2–3 exchanges, one drag-dropped + upload (`test.png`), one generated output. Right-click the chat in the + sidebar → Duplicate. + **Expect:** new chat ` (Copy)` with EVERY message; image embeds + render; the picker shows the copied files; downloads work in the copy. +- **11. Copy independence.** Delete the ORIGINAL chat, then reopen the copy. + **Expect:** embeds still render, files still download — bytes were + physically copied and references re-pointed. (Blob orphaning from the + original's delete is invisible here — **[KNOWN #2]** server-side.) +- **12. Branch fork with a cut.** In a chat where an upload arrived at + message 1 and an output was generated after message 3: click Fork on the + FIRST assistant reply. + **Expect:** new chat `Fork | ` containing only messages up to and + including that reply; the upload IS present; the post-cut output is NOT; + no ghost chips in the picker for files the fork doesn't have. +- **13. Fork the just-streamed reply.** Send a message, and the moment the + reply finishes, click Fork on it. + **Expect:** fork opens normally (the pre-fix bug 400'd on the synthetic + live-message id). +- **14. Fork/duplicate titles.** Fork the fork from test 12; duplicate it too. + **Expect:** titles stay sane — `Fork | ` (never `Fork | Fork | …`), + ` (Copy)`. +- **15. Rapid double duplicate.** Right-click → Duplicate twice in quick + succession. + **Expect:** two distinct copies, distinct names, no errors. +- **16. Quota accounting.** Check storage usage (settings) before/after + duplicating the media-heavy chat. + **Expect:** usage increased by the copied bytes (deliberate). Deleting the + copy does NOT refund — **[KNOWN #1 — observe, don't file]**. + +## Part C — Fixed-bug regression checks (rounds 1–6) + +- **17. Reference image is actually used.** Drag-drop a distinctive photo + into chat → *"Generate an image using my uploaded photo as reference, but + with a blue sky."* + **Expect:** the result visibly derives from the reference. Pre-fix, + generation silently ignored it and used the prompt alone. +- **18. Unresolvable reference fails loudly.** *"Generate an image using + uploads/does-not-exist.png as reference."* + **Expect:** the tool call FAILS with a clear error — not a prompt-only + generation. +- **19. UUID attachment ids resolve (round-6 fix).** With the drag-dropped + upload from test 17 (presign path → UUID row id): *"Convert my uploaded + image to grayscale with ffmpeg."* + **Expect:** the tool resolves the attachment and runs. Pre-fix, ids not + starting with `wf_` never resolved. +- **20. Failed tool calls surface their error.** *"Read the file + files/definitely-not-real-xyz.md."* + **Expect:** the tool block shows the actual error text and the model + responds to it sensibly ("that file doesn't exist…"). Failure mode this + guards: bare `{}` result + the model blindly retrying many times. +- **21. The reconnect-wedge scenario, end to end.** From the home surface + (fresh chat), first message: *"Create outputs/wedge-test.md with 'x'."* + (Same R8 exposure as test 2 — if the agent lands in files/ instead of + rendering an outputs/ link, use test 2's function_execute workaround + phrasing; the wedge scenario needs a real outputs/ link to click.) + When the reply renders the outputs/ link, click it, then send a second + message. + **Expect:** the tab opens AND the second send succeeds immediately. + Pre-fix this minted an empty-id resource, 400'd the send, and wedged the + UI in "running" for ~3 minutes. +- **22. Failed send rolls back.** Stop the mothership dev process (or go + offline) → send a message. + **Expect:** prompt error ("Failed to send message"), the optimistic + message is removed, UI returns to idle — no endless reconnect spinner. + Restart the backend; sending works again. (If a POST dies AFTER the server + accepted it, a duplicate assistant run is possible — **[KNOWN R0]**.) +- **23. Resume after refresh.** Ask for a long answer; refresh the page + mid-stream. + **Expect:** the stream resumes/reattaches and the reply completes — the + terminal-404 change must not have broken live resume. + +## Part D — Touched-surface regressions (old behavior, unchanged) + +- **24. Plain chat.** Multi-turn conversation with no files: send, receive, + Stop mid-stream, send again. All normal. +- **25. Workspace file tools.** *"Create files/reg-test.md with 'hi'"*, then + rename it, read it, delete it — all four verbs work as before. +- **26. Files page.** Upload `big.csv` via the Files UI; open it: the CSV + table preview loads (>5MB workspace path must still work). Open a + workspace markdown file: it IS still editable — the round-6 read-only + gate must apply ONLY to chat outputs. +- **27. open_resource for workspace assets.** *"Open files/reg-test.md"*, + *"open workflow "*, *"open table "* — tabs open for each. +- **28. Table import from a chat upload.** Drag-drop `small.csv` into chat → + *"Import this CSV into a new table."* + **Expect:** table created with the rows (chat-scoped storage context is + honored through the background import). +- **29. Knowledge add.** *"Add files/reg-test.md to knowledge base "* + and *"add outputs/notes.md to knowledge base "* — both ingest. +- **30. Resource picker, non-file types.** Via "+": add a workflow, a table, + a knowledge base; reorder tabs; remove them. All fine. Clicking a + file-FOLDER entry 400s — **[KNOWN #11 — observe, don't file]**. +- **31. @-mention.** Type `@` and mention a workspace file — works. Chat + outputs do NOT appear in @-mention — **[KNOWN R4]** (they do appear in + the "+" picker; that asymmetry is the tracked bug). +- **32. VFS over workspace files.** *"List the files in files/"*, *"search + my files for 'hi'"* — glob/grep/read on the workspace namespace normal. + `glob("outputs/*.png")` returning ALL outputs regardless of pattern is + **[KNOWN #6]**. +- **33. Doc images.** Edit a doc that references a workspace image via + edit_content. + **Expect:** images stage/compile as before. An outputs/-referenced image + silently dropping is **[KNOWN R7]**. + +## Part E — Flag, permissions, and obscure edges + +- **34. Flag OFF.** Disable `chat-scoped-outputs` in mothership → new chat → + *"Generate an image of a green square."* + **Expect:** pre-PR behavior (agent isn't steered to outputs/); normal chat + works end to end. A hallucinated explicit `outputs/…` write still creating + a chat-scoped output is the ACCEPTED flag-off behavior (see ISSUES.md + "Accepted"), not a bug. +- **35. Cross-user output access.** As user B (same workspace): copy the + serve/download URL of user A's chat output and open it. + **Expect:** denied/404 — outputs are private to the owning chat's user. + B's own chats and workspace files unaffected. +- **36. Namespace shadowing.** Create a real workspace folder literally + named `outputs` with a file in it. In a chat, *"read outputs/"*. + **Expect:** resolves against the CHAT namespace and misses — + **[KNOWN #8]**; the canonical `files/outputs/` spelling works. +- **37. Uploads/ create guard.** *"Create a file uploads/guard-test.md."* + **Expect (current, tracked):** falls through to `files/` prefixing and the + agent may create a literal `uploads` workspace folder — **[KNOWN R3 — + observe, don't file]**. (The same request with `outputs/` is properly + rejected — that's test 5's guard.) +- **38. Legacy empty-id resource cleanup.** If any chat predating the branch + carries a broken resource chip (empty id): removing it still works — the + remove contract stayed permissive on purpose. +- **39. Delete a chat with outputs.** Delete a chat that generated outputs. + **Expect:** chat gone, no client errors. Server-side blob orphaning is + **[KNOWN #2]** — nothing observable here; listed for completeness. + +## Result log + +Track results as: `#N PASS` / `#N FAIL — ` / `#N SKIP`. +Anything that fails outside the **[KNOWN …]** markers is a new finding — +add it to ISSUES.md with the file/mechanism if identifiable. diff --git a/apps/sim/app/api/copilot/chat/stream/route.ts b/apps/sim/app/api/copilot/chat/stream/route.ts index bd7f5465685..55d2f6e5a60 100644 --- a/apps/sim/app/api/copilot/chat/stream/route.ts +++ b/apps/sim/app/api/copilot/chat/stream/route.ts @@ -197,7 +197,13 @@ async function handleResumeRequestBody({ rootSpan: Span rootContext: Context }) { + // A transient DB error must NOT masquerade as "stream not found": the + // client treats 404 as terminal (StreamNotFoundError latches and kills all + // future reconnects for the stream), so 404 is reserved for a true miss and + // lookup failures return 503, which the client retries like any error. + let runLookupFailed = false const run = await getLatestRunForStream(streamId, authenticatedUserId).catch((err) => { + runLookupFailed = true logger.warn('Failed to fetch latest run for stream', { streamId, error: getErrorMessage(err), @@ -211,6 +217,11 @@ async function handleResumeRequestBody({ hasRun: !!run, runStatus: run?.status, }) + if (runLookupFailed) { + markSpanForError(rootSpan, 'Stream run lookup failed') + rootSpan.end() + return NextResponse.json({ error: 'Stream lookup failed' }, { status: 503 }) + } if (!run) { rootSpan.setAttribute(TraceAttr.CopilotResumeOutcome, CopilotResumeOutcome.StreamNotFound) rootSpan.end() diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts index 4409108eacd..054243b6b65 100644 --- a/apps/sim/app/api/files/authorization.test.ts +++ b/apps/sim/app/api/files/authorization.test.ts @@ -210,3 +210,69 @@ describe('public-context access (profile-pictures / og-images / workspace-logos) expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() }) }) + +/** + * Chat-scoped `output` files belong to a private chat: workspace membership + * alone must NOT grant access — the caller must also be the row's owner. + * These lock in the fix for the member-reads-another-member's-outputs leak, + * on both the explicit-context path (view route) and the inferred-workspace + * path (serve route — output keys share the workspace key shape). + */ +describe('chat output file ownership (verifyFileAccess)', () => { + const OUTPUT_KEY = 'workspace/ws-1/1780000000-abcd-generated-image.png' + const outputRow = { + workspaceId: 'ws-1', + userId: 'owner-user', + context: 'output', + deletedAt: null, + } + + beforeEach(() => { + vi.clearAllMocks() + }) + + it('denies a workspace member who is not the owning chat user (explicit output context)', async () => { + mockGetFileMetadataByKey.mockResolvedValue(outputRow) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect(verifyFileAccess(OUTPUT_KEY, 'other-member', undefined, 'output')).resolves.toBe( + false + ) + }) + + it('denies a non-owner member when context is inferred as workspace (serve path)', async () => { + mockGetFileMetadataByKey.mockResolvedValue(outputRow) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect( + verifyFileAccess(OUTPUT_KEY, 'other-member', undefined, 'workspace') + ).resolves.toBe(false) + }) + + it('grants the owning chat user with workspace membership', async () => { + mockGetFileMetadataByKey.mockResolvedValue(outputRow) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect(verifyFileAccess(OUTPUT_KEY, 'owner-user', undefined, 'output')).resolves.toBe( + true + ) + }) + + it('still denies the owner without workspace membership (left the workspace)', async () => { + mockGetFileMetadataByKey.mockResolvedValue(outputRow) + mockGetUserEntityPermissions.mockResolvedValue(null) + + await expect(verifyFileAccess(OUTPUT_KEY, 'owner-user', undefined, 'output')).resolves.toBe( + false + ) + }) + + it('leaves plain workspace files on membership-only auth', async () => { + mockGetFileMetadataByKey.mockResolvedValue({ ...outputRow, context: 'workspace' }) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect( + verifyFileAccess(OUTPUT_KEY, 'other-member', undefined, 'workspace') + ).resolves.toBe(true) + }) +}) diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 8e0c66b4173..240476b3742 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -15,6 +15,23 @@ import { isUuid } from '@/executor/constants' const logger = createLogger('FileAuthorization') +/** + * Contexts this key lookup resolves for workspace-membership authorization: + * `workspace` (durable UI files) and `output` (chat-scoped agent outputs). Both + * share the `workspace//...` key shape and are authorized by workspace + * membership, so a by-key match on either is safe to grant to any member. + * + * `mothership` chat uploads ALSO share the bucket/key shape but are deliberately + * left OUT: today they authorize via the Priority-2 object-metadata fallback, and + * moving them onto this Priority-1 DB path would change how uploads work — an + * unrelated change deferred to its own PR (see outputs-vfs-followups.md #2). + * Owner-scoped contexts that also live in `workspace_files` (`copilot`, + * `knowledge-base`, `workspace-logos`) are excluded too: they have stricter + * per-owner rules and must never be granted through workspace membership on a + * key collision. + */ +const WORKSPACE_FILE_LOOKUP_CONTEXTS: StorageContext[] = ['workspace', 'output'] + /** Thrown by utility functions when file access is denied, so route handlers can return 404. */ export class FileAccessDeniedError extends Error { constructor() { @@ -43,6 +60,21 @@ function workspacePermissionSatisfies( return permissionSatisfies(permission, requireWrite ? 'write' : 'read') } +/** + * Chat-scoped `output` rows belong to a PRIVATE chat: workspace membership + * alone is not enough — the caller must also be the row's owner (stamped from + * the chat owner at creation and re-stamped on fork/duplicate). Without this, + * any workspace member who learns a file id or storage key could read another + * member's agent outputs, even though listing outputs requires chat ownership. + * Non-output contexts pass through unchanged. + */ +function outputOwnershipSatisfied( + record: { context: string; uploadedBy: string }, + userId: string +): boolean { + return record.context !== 'output' || record.uploadedBy === userId +} + /** * Lookup workspace file by storage key from database * @param key Storage key to lookup @@ -51,16 +83,26 @@ function workspacePermissionSatisfies( async function lookupWorkspaceFileByKey( key: string, options?: { includeDeleted?: boolean } -): Promise<{ workspaceId: string; uploadedBy: string } | null> { +): Promise<{ workspaceId: string; uploadedBy: string; context: string } | null> { try { const { includeDeleted = false } = options ?? {} - // Priority 1: Check new workspaceFiles table - const fileRecord = await getFileMetadataByKey(key, 'workspace', { includeDeleted }) + // Priority 1: Check new workspaceFiles table. Look up by key across + // WORKSPACE_FILE_LOOKUP_CONTEXTS (`workspace` + `output`): both share the + // `workspace//...` key shape. `workspace` rows authorize by workspace + // membership; `output` rows additionally require ownership (see + // outputOwnershipSatisfied). Filtering to `workspace` alone made `output` + // files unservable (broken previews); scoping to this explicit set (rather + // than dropping the filter) keeps outputs servable while leaving upload + // (`mothership`) authorization and the owner-scoped contexts untouched. + const fileRecord = await getFileMetadataByKey(key, WORKSPACE_FILE_LOOKUP_CONTEXTS, { + includeDeleted, + }) if (fileRecord) { return { workspaceId: fileRecord.workspaceId || '', uploadedBy: fileRecord.userId, + context: fileRecord.context, } } @@ -83,6 +125,7 @@ async function lookupWorkspaceFileByKey( return { workspaceId: legacyFile.workspaceId, uploadedBy: legacyFile.uploadedBy, + context: 'workspace', } } } catch (legacyError) { @@ -157,8 +200,15 @@ export async function verifyFileAccess( return true } - // 1. Workspace / mothership files: Check database first (most reliable for both local and cloud) - if (inferredContext === 'workspace' || inferredContext === 'mothership') { + // 1. Workspace / mothership / chat-output files: check database first (most + // reliable for both local and cloud). `output` shares the workspace key + // shape; explicitly routing it here (instead of letting it fall through to + // verifyRegularFileAccess) keeps its ownership rule applied on every path. + if ( + inferredContext === 'workspace' || + inferredContext === 'mothership' || + inferredContext === 'output' + ) { return await verifyWorkspaceFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite) } @@ -218,6 +268,14 @@ async function verifyWorkspaceFileAccess( // Priority 1: Check database (most reliable, works for both local and cloud) const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey) if (workspaceFileRecord) { + if (!outputOwnershipSatisfied(workspaceFileRecord, userId)) { + logger.warn('Chat output file access denied: caller is not the owning chat user', { + userId, + workspaceId: workspaceFileRecord.workspaceId, + cloudKey, + }) + return false + } const permission = await getUserEntityPermissions( userId, 'workspace', @@ -647,6 +705,14 @@ async function verifyRegularFileAccess( // This handles legacy files that might not have metadata const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey) if (workspaceFileRecord) { + if (!outputOwnershipSatisfied(workspaceFileRecord, userId)) { + logger.warn('Chat output file access denied: caller is not the owning chat user', { + userId, + workspaceId: workspaceFileRecord.workspaceId, + cloudKey, + }) + return false + } const permission = await getUserEntityPermissions( userId, 'workspace', diff --git a/apps/sim/app/api/files/view/[id]/route.ts b/apps/sim/app/api/files/view/[id]/route.ts index e9d68b86821..f3612617fd5 100644 --- a/apps/sim/app/api/files/view/[id]/route.ts +++ b/apps/sim/app/api/files/view/[id]/route.ts @@ -44,14 +44,16 @@ export const GET = withRouteHandler( return NextResponse.json({ error: 'Not found' }, { status: 404 }) } - // Only workspace-scoped files are embeddable/viewable here. Other contexts (e.g. chat-scoped - // `mothership` uploads) are not durable workspace artifacts; now that the caller is known to have - // access, reject with a legible 422 so the embed fails cleanly and the file agent can self-correct. - if (record.context !== 'workspace') { - logger.warn('Rejected non-workspace file view', { id, context: record.context }) + // Embeddable contexts: durable `workspace` files and chat-scoped `output` files + // (agent-generated one-offs the user previews inline before optionally saving to + // the workspace). Other contexts (e.g. `mothership` user uploads) are not embeddable + // here; now that the caller is known to have access, reject with a legible 422 so the + // embed fails cleanly and the file agent can self-correct. + if (record.context !== 'workspace' && record.context !== 'output') { + logger.warn('Rejected non-embeddable file view', { id, context: record.context }) return NextResponse.json( { - error: `File ${id} has context "${record.context}" and is not embeddable. Only workspace files can be viewed via /api/files/view. Save it into the workspace and reference the workspace copy.`, + error: `File ${id} has context "${record.context}" and is not embeddable. Only workspace and output files can be viewed via /api/files/view. Save it into the workspace and reference the workspace copy.`, }, { status: 422 } ) diff --git a/apps/sim/app/api/mothership/chats/[chatId]/fork/route.test.ts b/apps/sim/app/api/mothership/chats/[chatId]/fork/route.test.ts new file mode 100644 index 00000000000..4245d8fff1e --- /dev/null +++ b/apps/sim/app/api/mothership/chats/[chatId]/fork/route.test.ts @@ -0,0 +1,630 @@ +/** + * @vitest-environment node + */ +import { copilotHttpMock, copilotHttpMockFns } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockTransaction, + mockSelectRows, + mockCheckStorageQuota, + mockFilterForkableChatFiles, + mockListDuplicableChatFiles, + mockPlanChatFileCopies, + mockExecuteChatFileBlobCopies, + mockLoadCopilotChatMessages, + mockAppendCopilotChatMessages, + mockAssertActiveWorkspaceAccess, + mockFetchGo, + mockPublishStatusChanged, + mockCaptureServerEvent, + mockDeleteWhere, + mockRemoveChatResources, +} = vi.hoisted(() => ({ + mockTransaction: vi.fn(), + mockSelectRows: vi.fn(), + mockCheckStorageQuota: vi.fn(), + // Real (pure) cut semantics so tests drive selection through row.messageId: + // rows with a NULL/undefined messageId are kept in every fork. + mockFilterForkableChatFiles: vi.fn( + (rows: Array<{ messageId?: string | null }>, kept: ReadonlySet) => + rows.filter((row) => !row.messageId || kept.has(row.messageId)) + ), + mockListDuplicableChatFiles: vi.fn(), + mockPlanChatFileCopies: vi.fn(), + mockExecuteChatFileBlobCopies: vi.fn(), + mockLoadCopilotChatMessages: vi.fn(), + mockAppendCopilotChatMessages: vi.fn(), + mockAssertActiveWorkspaceAccess: vi.fn(), + mockFetchGo: vi.fn(), + mockPublishStatusChanged: vi.fn(), + mockCaptureServerEvent: vi.fn(), + mockDeleteWhere: vi.fn(), + mockRemoveChatResources: vi.fn(), +})) + +vi.mock('@sim/db', () => ({ + db: { + select: () => ({ + from: () => ({ + where: () => ({ + limit: () => mockSelectRows(), + }), + }), + }), + delete: () => ({ + where: mockDeleteWhere, + }), + transaction: mockTransaction, + }, +})) + +vi.mock('@sim/db/schema', () => ({ + copilotChats: { + id: 'copilotChats.id', + userId: 'copilotChats.userId', + type: 'copilotChats.type', + workspaceId: 'copilotChats.workspaceId', + title: 'copilotChats.title', + model: 'copilotChats.model', + resources: 'copilotChats.resources', + previewYaml: 'copilotChats.previewYaml', + planArtifact: 'copilotChats.planArtifact', + config: 'copilotChats.config', + }, + workspaceFiles: { + id: 'workspaceFiles.id', + }, +})) + +vi.mock('drizzle-orm', () => ({ + eq: vi.fn((field: unknown, value: unknown) => ({ type: 'eq', field, value })), + inArray: vi.fn((field: unknown, values: unknown) => ({ type: 'inArray', field, values })), +})) + +vi.mock('@/lib/copilot/resources/persistence', () => ({ + removeChatResources: mockRemoveChatResources, +})) + +vi.mock('@/lib/copilot/request/http', () => copilotHttpMock) + +vi.mock('@/lib/billing/storage', () => ({ + checkStorageQuota: mockCheckStorageQuota, +})) + +vi.mock('@/lib/copilot/chat/fork-chat-files', () => ({ + filterForkableChatFiles: mockFilterForkableChatFiles, + listDuplicableChatFiles: mockListDuplicableChatFiles, + planChatFileCopies: mockPlanChatFileCopies, + executeChatFileBlobCopies: mockExecuteChatFileBlobCopies, +})) + +vi.mock('@/lib/copilot/chat/lifecycle', () => ({ + loadCopilotChatMessages: mockLoadCopilotChatMessages, +})) + +vi.mock('@/lib/copilot/chat/messages-store', () => ({ + appendCopilotChatMessages: mockAppendCopilotChatMessages, +})) + +vi.mock('@/lib/copilot/chat-status', () => ({ + chatPubSub: { publishStatusChanged: mockPublishStatusChanged }, +})) + +vi.mock('@/lib/copilot/request/go/fetch', () => ({ + fetchGo: mockFetchGo, +})) + +vi.mock('@/lib/copilot/server/agent-url', () => ({ + getMothershipBaseURL: vi.fn().mockResolvedValue('http://mothership.test'), + getMothershipSourceEnvHeaders: vi.fn().mockReturnValue({}), +})) + +vi.mock('@/lib/core/config/env', () => ({ env: {} })) + +vi.mock('@/lib/posthog/server', () => ({ + captureServerEvent: mockCaptureServerEvent, +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + assertActiveWorkspaceAccess: mockAssertActiveWorkspaceAccess, + isWorkspaceAccessDeniedError: () => false, +})) + +import { POST } from '@/app/api/mothership/chats/[chatId]/fork/route' + +const OLD_FILE_ID = 'wf_oldfile' +const NEW_FILE_ID = 'wf_newfile' + +const parentRow = { + id: 'chat-1', + userId: 'user-1', + type: 'mothership', + workspaceId: 'ws-1', + title: 'Generate Logs', + model: 'claude-opus-4-8', + resources: [{ type: 'file', id: OLD_FILE_ID, title: 'cat.png' }], + previewYaml: null, + planArtifact: null, + config: null, +} + +const threeMessages = [ + { + id: 'msg-1', + role: 'user', + content: `See ![cat](/api/files/view/${OLD_FILE_ID})`, + timestamp: '2026-07-01T00:00:00.000Z', + }, + { + id: 'msg-2', + role: 'assistant', + content: 'Nice cat.', + timestamp: '2026-07-01T00:00:01.000Z', + }, + { + id: 'msg-3', + role: 'user', + content: 'A later message the fork must not keep.', + timestamp: '2026-07-01T00:00:02.000Z', + }, +] + +/** Chat rows inserted through the mock transaction, captured for title assertions. */ +let insertedChatRows: Array> = [] +/** tx.update(...).set(...) payloads, captured for resource-rewrite assertions. */ +let updatedChatRows: Array> = [] + +function makeTx() { + return { + insert: () => ({ + values: (v: Record) => { + insertedChatRows.push(v) + return { + returning: async () => [{ id: 'row-id', workspaceId: 'ws-1' }], + } + }, + }), + update: () => ({ + set: (v: Record) => { + updatedChatRows.push(v) + return { where: async () => undefined } + }, + }), + } +} + +function createRequest(chatId: string, body?: unknown) { + return new NextRequest(`http://localhost:3000/api/mothership/chats/${chatId}/fork`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body ?? { upToMessageId: 'msg-2' }), + }) +} + +function makeContext(chatId: string) { + return { params: Promise.resolve({ chatId }) } +} + +describe('POST /api/mothership/chats/[chatId]/fork', () => { + beforeEach(() => { + vi.clearAllMocks() + insertedChatRows = [] + updatedChatRows = [] + copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({ + userId: 'user-1', + isAuthenticated: true, + }) + mockSelectRows.mockResolvedValue([parentRow]) + mockListDuplicableChatFiles.mockResolvedValue([]) + mockCheckStorageQuota.mockResolvedValue({ allowed: true }) + mockLoadCopilotChatMessages.mockResolvedValue(threeMessages) + mockPlanChatFileCopies.mockResolvedValue({ + idMap: new Map(), + keyMap: new Map(), + blobTasks: [], + }) + mockExecuteChatFileBlobCopies.mockResolvedValue({ copied: 0, failed: 0, failedCopyIds: [] }) + mockAppendCopilotChatMessages.mockResolvedValue(undefined) + mockDeleteWhere.mockResolvedValue(undefined) + mockRemoveChatResources.mockResolvedValue(undefined) + mockAssertActiveWorkspaceAccess.mockResolvedValue(undefined) + mockFetchGo.mockResolvedValue({ ok: true }) + mockTransaction.mockImplementation(async (cb: (tx: unknown) => Promise) => + cb(makeTx()) + ) + }) + + it('rejects unauthenticated callers', async () => { + copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({ + userId: null, + isAuthenticated: false, + }) + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(401) + }) + + it('404s when the chat belongs to another user', async () => { + mockSelectRows.mockResolvedValue([{ ...parentRow, userId: 'someone-else' }]) + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(404) + expect(mockTransaction).not.toHaveBeenCalled() + }) + + it('404s for non-mothership chats', async () => { + mockSelectRows.mockResolvedValue([{ ...parentRow, type: 'copilot' }]) + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(404) + }) + + it('400s when upToMessageId is an empty string', async () => { + const res = await POST(createRequest('chat-1', { upToMessageId: '' }), makeContext('chat-1')) + expect(res.status).toBe(400) + expect(mockTransaction).not.toHaveBeenCalled() + }) + + it('400s when the message is not in the chat', async () => { + const res = await POST( + createRequest('chat-1', { upToMessageId: 'msg-unknown' }), + makeContext('chat-1') + ) + expect(res.status).toBe(400) + expect(mockTransaction).not.toHaveBeenCalled() + }) + + it('applies the timeline cut: kept message ids drive the file selection', async () => { + // One upload born pre-cut, one output born pre-cut, one output born + // post-cut, and one legacy row with no birth message. The single + // chat-owned read is cut in memory: everything but the post-cut row. + const preCutUpload = { id: 'wf_up', size: 1, context: 'mothership', messageId: 'msg-1' } + const preCutOutput = { id: 'wf_out', size: 1, context: 'output', messageId: 'msg-1' } + const postCutOutput = { id: 'wf_late', size: 1, context: 'output', messageId: 'msg-3' } + const legacyRow = { id: 'wf_legacy', size: 1, context: 'mothership', messageId: null } + mockListDuplicableChatFiles.mockResolvedValue([ + preCutUpload, + preCutOutput, + postCutOutput, + legacyRow, + ]) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(200) + + // The cut runs over the single read with the kept slice (inclusive of + // msg-2, excluding msg-3). + expect(mockListDuplicableChatFiles).toHaveBeenCalledTimes(1) + const filterCall = mockFilterForkableChatFiles.mock.calls[0] + expect(filterCall[1]).toEqual(new Set(['msg-1', 'msg-2'])) + + // The copy plan receives the cut set — pre-cut upload AND output, plus the + // legacy no-birthdate row; the post-cut output stays behind. + expect(mockPlanChatFileCopies.mock.calls[0][0].rows).toEqual([ + preCutUpload, + preCutOutput, + legacyRow, + ]) + + // The appended transcript is the same inclusive slice. + const appended = mockAppendCopilotChatMessages.mock.calls[0] + expect(appended[1].map((m: { id: string }) => m.id)).toEqual(['msg-1', 'msg-2']) + }) + + it('fails up front with the quota error when copied bytes would exceed the limit', async () => { + mockListDuplicableChatFiles.mockResolvedValue([ + { size: 600, workspaceId: 'ws-1' }, + { size: 400, workspaceId: 'ws-1' }, + ]) + mockCheckStorageQuota.mockResolvedValue({ allowed: false, error: 'Storage limit exceeded' }) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + + expect(res.status).toBe(400) + expect(mockCheckStorageQuota).toHaveBeenCalledWith('user-1', 1000) + expect(mockTransaction).not.toHaveBeenCalled() + expect(mockExecuteChatFileBlobCopies).not.toHaveBeenCalled() + }) + + it('excludes uncopyable rows (no workspaceId) from the quota sum', async () => { + // planChatFileCopies skips workspaceId-less legacy rows, so their bytes + // must not count against the gate. + mockListDuplicableChatFiles.mockResolvedValue([ + { size: 600, workspaceId: 'ws-1' }, + { size: 400 }, + ]) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + + expect(res.status).toBe(200) + expect(mockCheckStorageQuota).toHaveBeenCalledWith('user-1', 600) + }) + + it('skips the quota check entirely when no chat-owned rows are in the cut', async () => { + // The chat owns one file, but it was born after the fork point. + mockListDuplicableChatFiles.mockResolvedValue([ + { id: 'wf_late', size: 500, context: 'output', messageId: 'msg-3' }, + ]) + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(200) + expect(mockCheckStorageQuota).not.toHaveBeenCalled() + }) + + it('forks the chat: copies kept uploads, rewrites references, clones agent state', async () => { + const blobTasks = [ + { + sourceKey: 'workspace/ws-1/old-cat.png', + targetKey: 'workspace/ws-1/new-cat.png', + context: 'mothership', + fileName: 'cat.png', + contentType: 'image/png', + }, + ] + mockListDuplicableChatFiles.mockResolvedValue([ + { size: 100, messageId: 'msg-1', workspaceId: 'ws-1' }, + ]) + mockPlanChatFileCopies.mockResolvedValue({ + idMap: new Map([[OLD_FILE_ID, NEW_FILE_ID]]), + keyMap: new Map([['workspace/ws-1/old-cat.png', 'workspace/ws-1/new-cat.png']]), + blobTasks, + }) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + const body = await res.json() + + expect(res.status).toBe(200) + expect(body.success).toBe(true) + expect(typeof body.id).toBe('string') + + expect(mockCheckStorageQuota).toHaveBeenCalledWith('user-1', 100) + + // The real rewriter runs: the kept message's view-URL points at the copy. + const appended = mockAppendCopilotChatMessages.mock.calls[0] + expect(appended[0]).toBe(body.id) + expect(appended[1][0].content).toBe(`See ![cat](/api/files/view/${NEW_FILE_ID})`) + + expect(mockExecuteChatFileBlobCopies).toHaveBeenCalledWith(blobTasks, { + userId: 'user-1', + workspaceId: 'ws-1', + }) + + const goCall = mockFetchGo.mock.calls[0] + expect(goCall[0]).toBe('http://mothership.test/api/chats/fork') + const goBody = JSON.parse(goCall[1].body) + expect(goBody).toEqual({ + sourceChatId: 'chat-1', + newChatId: body.id, + upToMessageId: 'msg-2', + userId: 'user-1', + }) + + expect(mockPublishStatusChanged).toHaveBeenCalledWith({ + workspaceId: 'ws-1', + chatId: body.id, + type: 'created', + }) + expect(mockCaptureServerEvent).toHaveBeenCalledWith( + 'user-1', + 'task_forked', + { workspace_id: 'ws-1', source_chat_id: 'chat-1', whole_chat: false }, + { groups: { workspace: 'ws-1' } } + ) + + // Branch forks are titled "Fork | ". + expect(insertedChatRows[0].title).toBe('Fork | Generate Logs') + }) + + it('still succeeds when the copilot-service clone fails (best-effort)', async () => { + mockFetchGo.mockRejectedValue(new Error('mothership unreachable')) + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + expect(res.status).toBe(200) + }) + + it('surfaces failed blob copies and cleans up their dead rows + resource chips', async () => { + mockExecuteChatFileBlobCopies.mockResolvedValue({ + copied: 1, + failed: 2, + failedCopyIds: ['wf_dead1', 'wf_dead2'], + }) + + const failedRes = await POST(createRequest('chat-1'), makeContext('chat-1')) + const body = await failedRes.json() + + expect(body.failedFileCopies).toBe(2) + + // The dead rows (committed, but no bytes behind them) are hard-deleted so + // they vanish from VFS listings and name resolution… + expect(mockDeleteWhere).toHaveBeenCalledWith({ + type: 'inArray', + field: 'workspaceFiles.id', + values: ['wf_dead1', 'wf_dead2'], + }) + // …and their resource chips are dropped from the new chat. + expect(mockRemoveChatResources).toHaveBeenCalledWith(body.id, [ + { type: 'file', id: 'wf_dead1', title: '' }, + { type: 'file', id: 'wf_dead2', title: '' }, + ]) + }) + + it('omits failedFileCopies and skips cleanup when every blob copies', async () => { + mockExecuteChatFileBlobCopies.mockResolvedValue({ copied: 3, failed: 0, failedCopyIds: [] }) + + const cleanRes = await POST(createRequest('chat-1'), makeContext('chat-1')) + + expect('failedFileCopies' in (await cleanRes.json())).toBe(false) + expect(mockDeleteWhere).not.toHaveBeenCalled() + expect(mockRemoveChatResources).not.toHaveBeenCalled() + }) + + it('copies pre-cut outputs and drops only post-cut ghosts on a branch fork', async () => { + // The source chat generated two outputs (apple pre-cut, banana post-cut) + // and has one upload + one shared workspace-file resource. A branch fork + // copies the kept upload AND the pre-cut output; only the post-cut output + // stays behind, so only its resource is dropped — not left pointing at + // the source chat. + mockSelectRows.mockResolvedValue([ + { + ...parentRow, + resources: [ + { type: 'file', id: OLD_FILE_ID, title: 'cat.png' }, + { type: 'file', id: 'wf_apple_output', title: 'apple.png' }, + { type: 'file', id: 'wf_banana_output', title: 'banana.png' }, + { type: 'file', id: 'wf_shared', title: 'shared.pdf' }, + { type: 'workflow', id: 'wflow-1', title: 'My flow' }, + ], + }, + ]) + // Every chat-owned file of the source chat (uploads + outputs) in the + // single read; messageId drives the in-memory cut. + mockListDuplicableChatFiles.mockResolvedValue([ + { id: OLD_FILE_ID, size: 100, context: 'mothership', messageId: 'msg-1' }, + { id: 'wf_apple_output', size: 50, context: 'output', messageId: 'msg-1' }, + { id: 'wf_banana_output', size: 50, context: 'output', messageId: 'msg-3' }, + ]) + mockPlanChatFileCopies.mockResolvedValue({ + idMap: new Map([ + [OLD_FILE_ID, NEW_FILE_ID], + ['wf_apple_output', 'wf_apple_copy'], + ]), + keyMap: new Map(), + blobTasks: [], + }) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + + expect(res.status).toBe(200) + // The plan received the cut set: upload + pre-cut output only. + expect(mockPlanChatFileCopies.mock.calls[0][0].rows.map((r: { id: string }) => r.id)).toEqual([ + OLD_FILE_ID, + 'wf_apple_output', + ]) + expect(updatedChatRows).toHaveLength(1) + expect(updatedChatRows[0].resources).toEqual([ + { type: 'file', id: NEW_FILE_ID, title: 'cat.png' }, + { type: 'file', id: 'wf_apple_copy', title: 'apple.png' }, + { type: 'file', id: 'wf_shared', title: 'shared.pdf' }, + { type: 'workflow', id: 'wflow-1', title: 'My flow' }, + ]) + }) + + it('drops ghosts even when the fork copies no files at all', async () => { + // Fork cut before the chat's only output was generated: the old guard + // skipped the resources update entirely when idMap was empty, leaving + // the ghost in place. + mockSelectRows.mockResolvedValue([ + { + ...parentRow, + resources: [{ type: 'file', id: 'wf_banana_output', title: 'banana.png' }], + }, + ]) + mockListDuplicableChatFiles.mockResolvedValue([ + { id: 'wf_banana_output', size: 50, context: 'output', messageId: 'msg-3' }, + ]) + + const res = await POST(createRequest('chat-1'), makeContext('chat-1')) + + expect(res.status).toBe(200) + expect(updatedChatRows).toHaveLength(1) + expect(updatedChatRows[0].resources).toEqual([]) + }) + + describe('whole-chat duplicate (no upToMessageId)', () => { + it('keeps every message and copies files with no timeline cut', async () => { + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + const body = await res.json() + + expect(res.status).toBe(200) + expect(body.success).toBe(true) + + // The single chat-owned read runs; a whole-chat duplicate never cuts it. + expect(mockListDuplicableChatFiles).toHaveBeenCalledTimes(1) + expect(mockListDuplicableChatFiles.mock.calls[0][1]).toBe('chat-1') + expect(mockFilterForkableChatFiles).not.toHaveBeenCalled() + + // Every message is appended — including msg-3, which a branch would cut. + const appended = mockAppendCopilotChatMessages.mock.calls[0] + expect(appended[1].map((m: { id: string }) => m.id)).toEqual(['msg-1', 'msg-2', 'msg-3']) + }) + + it('titles the copy " (Copy)"', async () => { + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + expect(res.status).toBe(200) + expect(insertedChatRows[0].title).toBe('Generate Logs (Copy)') + }) + + it('asks the copilot service for whole-chat mode (no upToMessageId in the body)', async () => { + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + const body = await res.json() + expect(res.status).toBe(200) + + const goBody = JSON.parse(mockFetchGo.mock.calls[0][1].body) + expect(goBody).toEqual({ + sourceChatId: 'chat-1', + newChatId: body.id, + userId: 'user-1', + }) + expect('upToMessageId' in goBody).toBe(false) + + expect(mockCaptureServerEvent).toHaveBeenCalledWith( + 'user-1', + 'task_forked', + { workspace_id: 'ws-1', source_chat_id: 'chat-1', whole_chat: true }, + { groups: { workspace: 'ws-1' } } + ) + }) + + it('gates the quota on the full upload + output byte total', async () => { + mockListDuplicableChatFiles.mockResolvedValue([ + { size: 700, context: 'mothership', workspaceId: 'ws-1' }, + { size: 500, context: 'output', workspaceId: 'ws-1' }, + ]) + mockCheckStorageQuota.mockResolvedValue({ allowed: false, error: 'Storage limit exceeded' }) + + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + + expect(res.status).toBe(400) + expect(mockCheckStorageQuota).toHaveBeenCalledWith('user-1', 1200) + expect(mockTransaction).not.toHaveBeenCalled() + }) + + it('duplicates an empty chat cleanly', async () => { + mockLoadCopilotChatMessages.mockResolvedValue([]) + + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + + expect(res.status).toBe(200) + expect(mockAppendCopilotChatMessages.mock.calls[0][1]).toEqual([]) + }) + + it('keeps every resource: all chat-owned files are copied, so none are ghosts', async () => { + mockSelectRows.mockResolvedValue([ + { + ...parentRow, + resources: [ + { type: 'file', id: OLD_FILE_ID, title: 'cat.png' }, + { type: 'file', id: 'wf_banana_output', title: 'banana.png' }, + ], + }, + ]) + mockListDuplicableChatFiles.mockResolvedValue([ + { id: OLD_FILE_ID, size: 100 }, + { id: 'wf_banana_output', size: 200 }, + ]) + mockPlanChatFileCopies.mockResolvedValue({ + idMap: new Map([ + [OLD_FILE_ID, NEW_FILE_ID], + ['wf_banana_output', 'wf_banana_copy'], + ]), + keyMap: new Map(), + blobTasks: [], + }) + + const res = await POST(createRequest('chat-1', {}), makeContext('chat-1')) + + expect(res.status).toBe(200) + expect(updatedChatRows[0].resources).toEqual([ + { type: 'file', id: NEW_FILE_ID, title: 'cat.png' }, + { type: 'file', id: 'wf_banana_copy', title: 'banana.png' }, + ]) + }) + }) +}) diff --git a/apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts b/apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts index 44b63c7f163..1a734fe38fa 100644 --- a/apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts +++ b/apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts @@ -1,13 +1,24 @@ import { db } from '@sim/db' -import { copilotChats } from '@sim/db/schema' +import { copilotChats, workspaceFiles } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' -import { eq } from 'drizzle-orm' +import { eq, inArray } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { forkMothershipChatContract } from '@/lib/api/contracts/mothership-chats' import { parseRequest } from '@/lib/api/server' +import { checkStorageQuota } from '@/lib/billing/storage' +import { + executeChatFileBlobCopies, + filterForkableChatFiles, + listDuplicableChatFiles, + planChatFileCopies, +} from '@/lib/copilot/chat/fork-chat-files' import { loadCopilotChatMessages } from '@/lib/copilot/chat/lifecycle' import { appendCopilotChatMessages } from '@/lib/copilot/chat/messages-store' +import { + rewriteMessageFileRefs, + rewriteResourceFileRefs, +} from '@/lib/copilot/chat/rewrite-file-references' import { chatPubSub } from '@/lib/copilot/chat-status' import { fetchGo } from '@/lib/copilot/request/go/fetch' import { @@ -18,6 +29,7 @@ import { createNotFoundResponse, createUnauthorizedResponse, } from '@/lib/copilot/request/http' +import { removeChatResources } from '@/lib/copilot/resources/persistence' import type { MothershipResource } from '@/lib/copilot/resources/types' import { getMothershipBaseURL, getMothershipSourceEnvHeaders } from '@/lib/copilot/server/agent-url' import { env } from '@/lib/core/config/env' @@ -32,8 +44,26 @@ const logger = createLogger('ForkChatAPI') /** * POST /api/mothership/chats/[chatId]/fork - * Creates a new chat branched from the given chat, keeping messages up to and - * including the specified message. Resources and copilot-side state are copied. + * Creates a new chat copied from the given chat, in one of two modes. + * + * Branch (upToMessageId set): keeps messages up to and including the specified + * message, along with the chat's uploads AND agent-generated outputs born + * at-or-before the fork point (a file travels iff the user message that + * carried/requested it is kept). The copy is titled "Fork | ". + * + * Whole-chat duplicate (upToMessageId omitted): keeps every message, copies + * every upload and output with no cut, titles the copy " (Copy)", and + * asks the copilot service for its whole-chat clone mode (compacted working + * memory preserved verbatim — nothing is cut, so nothing can leak across a + * cut). + * + * In both modes every copied file gets a fresh row id and storage key, bytes + * are physically copied and counted against the storage quota, and every + * in-transcript file reference is re-pointed at the copies so the new chat + * survives deletion of the source chat. File resources whose chat-owned file + * was NOT copied (a branch fork leaves post-cut uploads/outputs behind) are + * dropped from the new chat's resources rather than left as ghosts pointing + * at the source chat's files. */ export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ chatId: string }> }) => { @@ -43,12 +73,11 @@ export const POST = withRouteHandler( return createUnauthorizedResponse() } - const parsed = await parseRequest(forkMothershipChatContract, request, context, { - validationErrorResponse: () => createBadRequestResponse('upToMessageId is required'), - }) + const parsed = await parseRequest(forkMothershipChatContract, request, context) if (!parsed.success) return parsed.response const { chatId } = parsed.data.params const { upToMessageId } = parsed.data.body + const isWholeChatDuplicate = !upToMessageId const [parent] = await db .select({ @@ -76,23 +105,58 @@ export const POST = withRouteHandler( } const messages = await loadCopilotChatMessages(chatId) - const forkIdx = messages.findIndex((m) => m.id === upToMessageId) - if (forkIdx < 0) { - return createBadRequestResponse('Message not found in chat') + let forkedMessages = messages + if (upToMessageId) { + const forkIdx = messages.findIndex((m) => m.id === upToMessageId) + if (forkIdx < 0) { + return createBadRequestResponse('Message not found in chat') + } + forkedMessages = messages.slice(0, forkIdx + 1) } - const forkedMessages = messages.slice(0, forkIdx + 1) - // Resources are stored as a jsonb array on the chat row — copy them directly. + // Single workspace_files read per fork: every chat-owned file (uploads + + // outputs). A whole-chat duplicate copies all of them; a branch fork + // timeline-cuts to the kept message slice in memory (files born after + // the fork point stay behind). + const chatOwnedFiles = await listDuplicableChatFiles(db, chatId) + const sourceFiles = isWholeChatDuplicate + ? chatOwnedFiles + : filterForkableChatFiles(chatOwnedFiles, new Set(forkedMessages.map((m) => m.id))) + // Sum only rows the plan will actually copy — planChatFileCopies skips + // rows with no workspaceId, so counting their bytes could reject a fork + // whose real copies fit within quota. + const totalFileBytes = sourceFiles.reduce( + (sum, row) => (row.workspaceId ? sum + row.size : sum), + 0 + ) + if (totalFileBytes > 0) { + const quotaCheck = await checkStorageQuota(userId, totalFileBytes) + if (!quotaCheck.allowed) { + return createBadRequestResponse(quotaCheck.error || 'Storage limit exceeded') + } + } + + // Resources are stored as a jsonb array on the chat row. They carry no + // timestamps, so they can't be timeline-cut like messages — instead, + // file resources whose chat-owned file is NOT copied (uploads/outputs + // born after a branch fork's cut) are dropped in the rewrite below; + // everything else is copied. const parentResources = Array.isArray(parent.resources) ? (parent.resources as MothershipResource[]) : [] + // The source chat's chat-owned file ids (uploads + outputs, no cut) — + // the "is this resource a ghost?" test set for the rewrite. + const chatOwnedFileIds = new Set(chatOwnedFiles.map((row) => row.id)) + const newId = generateId() + // Both modes strip a leading "Fork | " so titles don't stack prefixes: + // duplicating a forked chat yields "Name (Copy)", not "Fork | Name (Copy)". const baseTitle = (parent.title ?? 'New chat').replace(/^Fork \| /, '') - const title = `Fork | ${baseTitle}` + const title = isWholeChatDuplicate ? `${baseTitle} (Copy)` : `Fork | ${baseTitle}` const now = new Date() - const newChat = await db.transaction(async (tx) => { + const result = await db.transaction(async (tx) => { const [row] = await tx .insert(copilotChats) .values({ @@ -114,15 +178,76 @@ export const POST = withRouteHandler( if (!row) return null - await appendCopilotChatMessages(newId, forkedMessages, { chatModel: parent.model }, tx) - return row + // File rows FK the new chat row, so the plan runs after the insert. + const { idMap, keyMap, blobTasks } = await planChatFileCopies({ + tx, + rows: sourceFiles, + newChatId: newId, + userId, + now, + }) + + const maps = { fileIds: idMap, fileKeys: keyMap } + const newChatResources = rewriteResourceFileRefs(parentResources, maps, chatOwnedFileIds) + // Skip the redundant update only when the rewrite changed nothing: + // no ids re-pointed AND no ghost resources dropped. (idMap and keyMap + // are populated in lockstep, so idMap alone decides the first half.) + if (idMap.size > 0 || newChatResources.length !== parentResources.length) { + await tx + .update(copilotChats) + .set({ resources: newChatResources }) + .where(eq(copilotChats.id, newId)) + } + + await appendCopilotChatMessages( + newId, + rewriteMessageFileRefs(forkedMessages, maps), + { chatModel: parent.model }, + tx + ) + return { row, blobTasks } }) - if (!newChat) { + if (!result) { return createInternalServerErrorResponse('Failed to create forked chat') } + const newChat = result.row + + const { copied, failed, failedCopyIds } = await executeChatFileBlobCopies(result.blobTasks, { + userId, + workspaceId: parent.workspaceId ?? undefined, + }) + if (failed > 0) { + // A failed blob copy leaves a committed row with no bytes behind it. + // Cleanly absent beats present-but-broken: hard-delete the dead rows + // (they vanish from the VFS listings and name resolution) and drop + // their resource chips from the new chat. Inline transcript embeds + // can't be healed — those 404 — which is what `failedFileCopies` in + // the response warns the user about. + try { + await db.delete(workspaceFiles).where(inArray(workspaceFiles.id, failedCopyIds)) + await removeChatResources( + newId, + failedCopyIds.map((id) => ({ type: 'file' as const, id, title: '' })) + ) + } catch (cleanupError) { + logger.error('Failed to clean up dead file rows after blob-copy failure', { + newChatId: newId, + failedCopyIds, + error: cleanupError, + }) + } + logger.warn('Some chat file blobs failed to copy during fork', { + chatId, + newChatId: newId, + copied, + failed, + }) + } // Clone copilot-service conversation state (messages, active_messages, memory files). + // Omitting upToMessageId selects the service's whole-chat mode, which preserves the + // compacted working memory verbatim instead of rebuilding it from raw messages. // Best-effort: if the copilot service doesn't have a row for the source chat yet, skip. try { const copilotHeaders: Record = { 'Content-Type': 'application/json' } @@ -137,7 +262,7 @@ export const POST = withRouteHandler( body: JSON.stringify({ sourceChatId: chatId, newChatId: newId, - upToMessageId, + ...(upToMessageId ? { upToMessageId } : {}), userId, }), spanName: 'sim → go /api/chats/fork', @@ -164,11 +289,19 @@ export const POST = withRouteHandler( captureServerEvent( userId, 'task_forked', - { workspace_id: parent.workspaceId ?? '', source_chat_id: chatId }, + { + workspace_id: parent.workspaceId ?? '', + source_chat_id: chatId, + whole_chat: isWholeChatDuplicate, + }, { groups: { workspace: parent.workspaceId ?? '' } } ) - return NextResponse.json({ success: true, id: newId }) + return NextResponse.json({ + success: true, + id: newId, + ...(failed > 0 ? { failedFileCopies: failed } : {}), + }) } catch (error) { if (isWorkspaceAccessDeniedError(error)) { return createForbiddenResponse('Workspace access denied') diff --git a/apps/sim/app/api/mothership/chats/[chatId]/outputs/route.ts b/apps/sim/app/api/mothership/chats/[chatId]/outputs/route.ts new file mode 100644 index 00000000000..fc3af6b1b51 --- /dev/null +++ b/apps/sim/app/api/mothership/chats/[chatId]/outputs/route.ts @@ -0,0 +1,51 @@ +import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' +import { type NextRequest, NextResponse } from 'next/server' +import { listChatOutputsContract } from '@/lib/api/contracts/mothership-chats' +import { parseRequest } from '@/lib/api/server' +import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle' +import { + authenticateCopilotRequestSessionOnly, + createUnauthorizedResponse, +} from '@/lib/copilot/request/http' +import { listChatOutputs } from '@/lib/copilot/tools/handlers/chat-file-reader' +import { withRouteHandler } from '@/lib/core/utils/with-route-handler' + +const logger = createLogger('MothershipChatOutputsAPI') + +/** + * GET /api/mothership/chats/[chatId]/outputs + * + * List the chat-scoped `output` files (agent-generated one-offs) for a chat. These + * never appear in the workspace Files list (`listWorkspaceFiles` is workspace-only), + * so the resource panel uses this to show them alongside workspace files in the + * "+" resource picker and open them as tabs. Returns the same `WorkspaceFileRecord` + * shape as the workspace file list. + */ +export const GET = withRouteHandler( + async (request: NextRequest, context: { params: Promise<{ chatId: string }> }) => { + try { + const { userId, isAuthenticated } = await authenticateCopilotRequestSessionOnly() + if (!isAuthenticated || !userId) { + return createUnauthorizedResponse() + } + + const parsed = await parseRequest(listChatOutputsContract, request, context) + if (!parsed.success) return parsed.response + const { chatId } = parsed.data.params + const chat = await getAccessibleCopilotChatAuth(chatId, userId) + if (!chat) { + return NextResponse.json({ success: false, error: 'Chat not found' }, { status: 404 }) + } + + const files = await listChatOutputs(chatId) + return NextResponse.json({ success: true, files }) + } catch (error) { + logger.error('Failed to list chat outputs', error) + return NextResponse.json( + { success: false, error: getErrorMessage(error, 'Failed to list chat outputs') }, + { status: 500 } + ) + } + } +) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.test.ts new file mode 100644 index 00000000000..e664efc5598 --- /dev/null +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.test.ts @@ -0,0 +1,114 @@ +/** + * Tests for the workspace CSV preview route. Pins the regression where the + * route resolved files via getWorkspaceFile (context='workspace' only), so a + * chat-scoped CSV output — resolvable by the same panel that opens the tab — + * permanently 404'd in CsvTablePreview. + * + * @vitest-environment node + */ + +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCheckSessionOrInternalAuth, + mockGetUserEntityPermissions, + mockGetPreviewable, + mockGetCsvPreviewSlice, +} = vi.hoisted(() => ({ + mockCheckSessionOrInternalAuth: vi.fn(), + mockGetUserEntityPermissions: vi.fn(), + mockGetPreviewable: vi.fn(), + mockGetCsvPreviewSlice: vi.fn(), +})) + +vi.mock('@/lib/auth/hybrid', () => ({ + checkSessionOrInternalAuth: mockCheckSessionOrInternalAuth, +})) + +vi.mock('@/lib/workspaces/permissions/utils', () => ({ + getUserEntityPermissions: mockGetUserEntityPermissions, +})) + +vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ + getPreviewableWorkspaceFile: mockGetPreviewable, +})) + +vi.mock('@/lib/file-parsers/csv-preview-slice', () => ({ + getCsvPreviewSlice: mockGetCsvPreviewSlice, +})) + +import { GET } from './route' + +const OUTPUT_KEY = 'chat/chat-1/data.csv' +const SLICE = { headers: ['a'], rows: [['1']], truncated: false } + +function createRequest(key: string) { + return new NextRequest( + `http://localhost:3000/api/workspaces/ws-1/files/wf_output/csv-preview?key=${encodeURIComponent(key)}` + ) +} + +const routeContext = () => ({ params: Promise.resolve({ id: 'ws-1', fileId: 'wf_output' }) }) + +describe('GET /api/workspaces/[id]/files/[fileId]/csv-preview', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckSessionOrInternalAuth.mockResolvedValue({ success: true, userId: 'user-1' }) + mockGetUserEntityPermissions.mockResolvedValue('read') + mockGetCsvPreviewSlice.mockResolvedValue(SLICE) + }) + + it('serves a chat-scoped CSV output from its own storage context', async () => { + mockGetPreviewable.mockResolvedValue({ + id: 'wf_output', + key: OUTPUT_KEY, + storageContext: 'output', + }) + + const response = await GET(createRequest(OUTPUT_KEY), routeContext()) + + expect(response.status).toBe(200) + expect(mockGetPreviewable).toHaveBeenCalledWith('ws-1', 'wf_output', 'user-1') + expect(mockGetCsvPreviewSlice).toHaveBeenCalledWith( + expect.objectContaining({ key: OUTPUT_KEY, context: 'output' }) + ) + }) + + it('serves a workspace CSV with the workspace context', async () => { + mockGetPreviewable.mockResolvedValue({ + id: 'wf_output', + key: OUTPUT_KEY, + storageContext: 'workspace', + }) + + const response = await GET(createRequest(OUTPUT_KEY), routeContext()) + + expect(response.status).toBe(200) + expect(mockGetCsvPreviewSlice).toHaveBeenCalledWith( + expect.objectContaining({ context: 'workspace' }) + ) + }) + + it('404s when the previewable lookup denies or misses (non-owner output, deleted file)', async () => { + mockGetPreviewable.mockResolvedValue(null) + + const response = await GET(createRequest(OUTPUT_KEY), routeContext()) + + expect(response.status).toBe(404) + expect(mockGetCsvPreviewSlice).not.toHaveBeenCalled() + }) + + it('404s when the client-supplied key does not match the record', async () => { + mockGetPreviewable.mockResolvedValue({ + id: 'wf_output', + key: OUTPUT_KEY, + storageContext: 'output', + }) + + const response = await GET(createRequest('some/other/key.csv'), routeContext()) + + expect(response.status).toBe(404) + expect(mockGetCsvPreviewSlice).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts index 0fd9553a4c4..0cab7a001fe 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/csv-preview/route.ts @@ -5,7 +5,7 @@ import { parseRequest } from '@/lib/api/server' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { getCsvPreviewSlice } from '@/lib/file-parsers/csv-preview-slice' -import { getWorkspaceFile } from '@/lib/uploads/contexts/workspace/workspace-file-manager' +import { getPreviewableWorkspaceFile } from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' const logger = createLogger('WorkspaceCsvPreviewAPI') @@ -33,15 +33,17 @@ export const GET = withRouteHandler( // Resolve the file record (active, in this workspace) and read from its authoritative key — // never the client-supplied one. This rejects archived/deleted files and keys with no live - // row, matching the access guarantees of /api/files/serve. - const record = await getWorkspaceFile(workspaceId, fileId) + // row, matching the access guarantees of /api/files/serve. Resolution goes through the + // previewable accessor (not getWorkspaceFile) so chat-scoped CSV outputs — which the resource + // panel resolves through the same accessor — preview too, with its owner-only gate applied. + const record = await getPreviewableWorkspaceFile(workspaceId, fileId, userId) if (!record || record.key !== key) { return NextResponse.json({ error: 'File not found' }, { status: 404 }) } const slice = await getCsvPreviewSlice({ key: record.key, - context: 'workspace', + context: record.storageContext ?? 'workspace', signal: request.signal, }) diff --git a/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.ts b/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.ts index 1826988ea08..33ef0996ac8 100644 --- a/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.ts +++ b/apps/sim/app/api/workspaces/[id]/files/[fileId]/route.ts @@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { type NextRequest, NextResponse } from 'next/server' import { + getWorkspaceFileByIdContract, renameWorkspaceFileContract, workspaceFileParamsSchema, } from '@/lib/api/contracts/workspace-files' @@ -10,6 +11,7 @@ import { getSession } from '@/lib/auth' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' +import { getPreviewableWorkspaceFile } from '@/lib/uploads/contexts/workspace' import { performDeleteWorkspaceFileItems, performRenameWorkspaceFile, @@ -20,6 +22,51 @@ export const dynamic = 'force-dynamic' const logger = createLogger('WorkspaceFileAPI') +/** + * GET /api/workspaces/[id]/files/[fileId] + * Fetch a single file record by id, including chat-scoped `output` files that never + * appear in the workspace Files list. Used by the resource panel to preview an output + * the list-based lookup can't see. Requires workspace membership (read). + */ +export const GET = withRouteHandler( + async (request: NextRequest, context: { params: Promise<{ id: string; fileId: string }> }) => { + const requestId = generateRequestId() + + try { + const session = await getSession() + if (!session?.user?.id) { + return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) + } + + const parsed = await parseRequest(getWorkspaceFileByIdContract, request, context) + if (!parsed.success) return parsed.response + const { id: workspaceId, fileId } = parsed.data.params + + const userPermission = await getUserEntityPermissions( + session.user.id, + 'workspace', + workspaceId + ) + if (userPermission === null) { + return NextResponse.json({ error: 'Insufficient permissions' }, { status: 403 }) + } + + const file = await getPreviewableWorkspaceFile(workspaceId, fileId, session.user.id) + if (!file) { + return NextResponse.json({ success: false, error: 'File not found' }, { status: 404 }) + } + + return NextResponse.json({ success: true, file }) + } catch (error) { + logger.error(`[${requestId}] Error fetching workspace file:`, error) + return NextResponse.json( + { success: false, error: getErrorMessage(error, 'Failed to fetch file') }, + { status: 500 } + ) + } + } +) + /** * PATCH /api/workspaces/[id]/files/[fileId] * Rename a workspace file (requires write permission) diff --git a/apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx b/apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx index 91ebc37ea42..36664e8c0ae 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx +++ b/apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx @@ -17,6 +17,7 @@ import { } from '@sim/emcn' import { GitBranch } from 'lucide-react' import { useParams, useRouter } from 'next/navigation' +import { isLiveAssistantMessageId } from '@/lib/copilot/chat/effective-transcript' import { useChatSurface } from '@/app/workspace/[workspaceId]/home/components/chat-surface-context' import { useSubmitCopilotFeedback } from '@/hooks/queries/copilot-feedback' import { useForkMothershipChat } from '@/hooks/queries/mothership-chats' @@ -153,6 +154,11 @@ export const MessageActions = memo(function MessageActions({ if (!chatId || !messageId || forkChat.isPending) return try { const result = await forkChat.mutateAsync({ chatId, upToMessageId: messageId }) + if (result.failedFileCopies) { + toast.warning( + `${result.failedFileCopies} file${result.failedFileCopies === 1 ? '' : 's'} could not be copied to the fork` + ) + } useFolderStore.getState().clearChatSelection() router.push(`/workspace/${params.workspaceId}/chat/${result.id}`) } catch { @@ -162,7 +168,10 @@ export const MessageActions = memo(function MessageActions({ const hasContent = Boolean(content) const canSubmitFeedback = Boolean(chatId && userQuery) - const canFork = false + // A live (just-streamed) assistant message carries a synthetic id that the + // persisted transcript doesn't know — forking it would 400. The button + // appears once the transcript refetch swaps in the persisted message id. + const canFork = Boolean(chatId && messageId && !isLiveAssistantMessageId(messageId)) if (!hasContent && !canSubmitFeedback && !canFork) return null return ( diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/add-resource-dropdown.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/add-resource-dropdown.tsx index 7f67d25704f..5624ca81fca 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/add-resource-dropdown.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/add-resource-dropdown.tsx @@ -35,7 +35,7 @@ import { useWorkspaceSchedules } from '@/hooks/queries/schedules' import { useTablesList } from '@/hooks/queries/tables' import { useWorkflows } from '@/hooks/queries/workflows' import { useWorkspaceFileFolders } from '@/hooks/queries/workspace-file-folders' -import { useWorkspaceFiles } from '@/hooks/queries/workspace-files' +import { useChatOutputs, useWorkspaceFiles } from '@/hooks/queries/workspace-files' export interface AddResourceDropdownProps { workspaceId: string @@ -44,6 +44,8 @@ export interface AddResourceDropdownProps { onSwitch?: (resourceId: string) => void /** Resource types to hide from the dropdown (e.g. `['folder', 'task']`). */ excludeTypes?: readonly MothershipResourceType[] + /** Active chat id; when set, this chat's `output` files are listed under Files too. */ + chatId?: string } export type AvailableItem = { id: string; name: string; isOpen?: boolean; [key: string]: unknown } @@ -70,11 +72,15 @@ const LOG_DROPDOWN_FILTERS = { export function useAvailableResources( workspaceId: string, existingKeys: Set, - excludeTypes?: readonly MothershipResourceType[] + excludeTypes?: readonly MothershipResourceType[], + chatId?: string ): AvailableItemsByType[] { const { data: workflows = [] } = useWorkflows(workspaceId) const { data: tables = [] } = useTablesList(workspaceId) const { data: files = [] } = useWorkspaceFiles(workspaceId) + // Chat-scoped agent outputs aren't in the workspace list; surface them here so a + // generated `outputs/` file is pickable in the "+" menu and openable as a tab. + const { data: chatOutputs = [] } = useChatOutputs(chatId) const { data: knowledgeBases } = useKnowledgeBasesQuery(workspaceId) const { data: folders = [] } = useFolders(workspaceId) const { data: fileFolders = [] } = useWorkspaceFileFolders(workspaceId) @@ -85,6 +91,11 @@ export function useAvailableResources( return useMemo(() => { const excluded = new Set(excludeTypes ?? []) + // Merge workspace files with this chat's outputs, deduped by id (a materialized + // output becomes a workspace file with the same id, so it must not appear twice). + const fileIds = new Set(files.map((f) => f.id)) + const fileList = + chatOutputs.length > 0 ? [...files, ...chatOutputs.filter((o) => !fileIds.has(o.id))] : files const groups: AvailableItemsByType[] = [ { type: 'workflow' as const, @@ -116,7 +127,7 @@ export function useAvailableResources( }, { type: 'file' as const, - items: files.map((f) => ({ + items: fileList.map((f) => ({ id: f.id, name: f.name, folderId: f.folderId ?? null, @@ -190,6 +201,7 @@ export function useAvailableResources( fileFolders, tables, files, + chatOutputs, knowledgeBases, tasks, schedules, @@ -387,14 +399,17 @@ export function AddResourceDropdown({ onAdd, onSwitch, excludeTypes, + chatId, }: AddResourceDropdownProps) { const [open, setOpen] = useState(false) const [search, setSearch] = useState('') const [activeIndex, setActiveIndex] = useState(0) - const available = useAvailableResources(workspaceId, existingKeys, [ - ...(excludeTypes ?? []), - 'integration', - ]) + const available = useAvailableResources( + workspaceId, + existingKeys, + [...(excludeTypes ?? []), 'integration'], + chatId + ) const handleOpenChange = (next: boolean) => { setOpen(next) if (!next) { diff --git a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx index 0bfe6bdb233..8cc74564316 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx +++ b/apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/resource-content.tsx @@ -56,7 +56,11 @@ import { useLogDetail } from '@/hooks/queries/logs' import { useScheduleById } from '@/hooks/queries/schedules' import { downloadTableExport } from '@/hooks/queries/tables' import { useWorkflows } from '@/hooks/queries/workflows' -import { useWorkspaceFiles } from '@/hooks/queries/workspace-files' +import { + useChatOutputs, + useWorkspaceFileById, + useWorkspaceFiles, +} from '@/hooks/queries/workspace-files' import { useSettingsNavigation } from '@/hooks/use-settings-navigation' import { useExecutionStore } from '@/stores/execution/store' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' @@ -280,9 +284,11 @@ export const ResourceContent = memo(function ResourceContent({ interface ResourceActionsProps { workspaceId: string resource: MothershipResource + /** Active chat id — lets file actions resolve this chat's outputs by name, like the preview. */ + chatId?: string } -export function ResourceActions({ workspaceId, resource }: ResourceActionsProps) { +export function ResourceActions({ workspaceId, resource, chatId }: ResourceActionsProps) { switch (resource.type) { case 'workflow': return @@ -292,6 +298,7 @@ export function ResourceActions({ workspaceId, resource }: ResourceActionsProps) workspaceId={workspaceId} fileId={resource.id} filePath={resource.path} + chatId={chatId} /> ) case 'knowledgebase': @@ -499,16 +506,23 @@ function EmbeddedTableActions({ workspaceId, tableId, tableName }: EmbeddedTable const fileLogger = createLogger('EmbeddedFileActions') -interface EmbeddedFileActionsProps { - workspaceId: string - fileId: string - filePath?: string -} - -function EmbeddedFileActions({ workspaceId, fileId, filePath }: EmbeddedFileActionsProps) { - const router = useRouter() - const { data: files = [] } = useWorkspaceFiles(workspaceId) - const file = useMemo( +/** + * Resolve an embedded file reference three ways, in order: the workspace + * Files list (by id or canonical path), a direct by-id fetch (covers + * chat-scoped `output` rows, which the list excludes), and finally this + * chat's outputs by leaf name (covers outputs referenced by an `outputs/` + * path with no resolvable id). Shared by the preview (EmbeddedFile) and its + * header actions (EmbeddedFileActions) so the two can never disagree about + * whether a file exists. + */ +function useResolvedEmbeddedFile( + workspaceId: string, + fileId: string, + filePath?: string, + chatId?: string +) { + const { data: files = [], isLoading, isFetching } = useWorkspaceFiles(workspaceId) + const listFile = useMemo( () => files.find( (f) => @@ -519,6 +533,52 @@ function EmbeddedFileActions({ workspaceId, fileId, filePath }: EmbeddedFileActi [files, fileId, filePath] ) + const listSettled = !isLoading && !isFetching + const { data: fallbackFile = null, isLoading: fallbackLoading } = useWorkspaceFileById( + workspaceId, + fileId, + listSettled && !listFile + ) + + // The agent may reference an output by PATH (e.g. an `outputs/generated-image.jpg` + // link) rather than by id, in which case both the list and the by-id fetch miss + // (by-id matches on the wf_ id, not a path). Resolve it against this chat's + // outputs by leaf name. + const { data: chatOutputs = [], isLoading: outputsLoading } = useChatOutputs(chatId) + const outputFallback = useMemo(() => { + if (listFile || fallbackFile) return null + const ref = filePath ?? (fileId || '') + if (!ref) return null + const rawLeaf = ref.split('/').pop() ?? '' + let leaf = rawLeaf + try { + leaf = decodeURIComponent(rawLeaf) + } catch { + leaf = rawLeaf + } + return chatOutputs.find((o) => o.id === fileId || o.name === leaf || o.name === rawLeaf) ?? null + }, [listFile, fallbackFile, chatOutputs, filePath, fileId]) + + const file = listFile ?? fallbackFile ?? outputFallback + const isResolving = + isLoading || + (isFetching && !file) || + (listSettled && !listFile && (fallbackLoading || outputsLoading)) + + return { file, isResolving } +} + +interface EmbeddedFileActionsProps { + workspaceId: string + fileId: string + filePath?: string + chatId?: string +} + +function EmbeddedFileActions({ workspaceId, fileId, filePath, chatId }: EmbeddedFileActionsProps) { + const router = useRouter() + const { file } = useResolvedEmbeddedFile(workspaceId, fileId, filePath, chatId) + const handleDownload = async () => { if (!file) return try { @@ -532,23 +592,29 @@ function EmbeddedFileActions({ workspaceId, fileId, filePath }: EmbeddedFileActi router.push(`/workspace/${workspaceId}/files/${encodeURIComponent(file?.id ?? fileId)}`) } + // The Files page lists only context='workspace' rows — navigating a + // chat-scoped output there dead-ends on the plain list, so hide the action. + const canOpenInFiles = !file?.storageContext || file.storageContext === 'workspace' + return ( <> - - - - - -

Open in files

- - + {canOpenInFiles && ( + + + + + +

Open in files

+ + + )}