Repository navigation
📥 feat: Import Chat Uploads Into Attached Workspaces - #348
Conversation
Add the additive `import_file` workspace operation so a chat's uploaded files, binaries included, land in an attached BYOM workspace. Code API verifies the caller owns the stored object with the download route's session rule, mints a one-object, read-only, short-lived egress grant, and relays opaque handles to the worker. The worker streams the object through its file relay upstream into a same-directory temporary file, enforces the declared size and SHA-256, refuses `.git` paths and symlinked parents, and installs it atomically as 0644. Identical bytes are an idempotent no-op; different bytes conflict unless `overwrite`. The operation is negotiated: Code API offers it only with an egress gateway, and workers advertise it only with writes enabled and a relay upstream configured.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3035b45977
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (target.state === 'different') { | ||
| await rename(temporary, installTarget); |
There was a problem hiding this comment.
When overwrite: true targets an existing file, another local process can replace that file after the preceding lstat(candidate) identity check but before this unconditional rename. The rename then silently overwrites the new inode rather than returning EDIT_CONFLICT, so a concurrent editor's save can be lost despite the operation's guarantee that it replaces only the inode it inspected. Use an atomic compare/exchange strategy or otherwise prevent a path replacement in this check-to-rename window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Narrowed rather than changed, in 528c5ea. There is no portable atomic compare-and-rename (RENAME_EXCHANGE is Linux-only and not exposed by the root-access layer), and this is the same check-then-rename window write_file/edit_file already have. Bridge mutations of one root are serialized by Code API and the worker guard, so only an out-of-band local writer can race the last step. The inode recheck now runs immediately before the rename, and the docs/comments no longer claim a stronger guarantee.
| let preparedImport: PreparedWorkspaceImport | undefined; | ||
| if (prepareImport != null) { | ||
| try { | ||
| preparedImport = await prepareImport(); |
There was a problem hiding this comment.
In the synchronous /workspace-tools/execute import path, ownership lookup and the egress-gateway request are awaited before the request/response abort listeners are installed. If the client disconnects during either await, both events have already fired by the time the listeners are attached, so dispatchWorkspaceTool receives a live signal and can still mutate the workspace after the caller cancelled. Install the listeners before the first import-specific await and also check the already-aborted/closed state before dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 528c5ea: abort listeners (plus an already-destroyed-socket check) are installed before ownership and grant minting, and a caller that left meanwhile returns before dispatch; its grant is still revoked in finally. Covered by a new disconnect test.
| if (entry == null) { | ||
| try { | ||
| await mkdir(current, 0o777); | ||
| await mkdir(current, mode); | ||
| created.push(current); |
There was a problem hiding this comment.
On POSIX systems the mode passed to mkdir/mkdirat is filtered through the process umask. A worker launched with a restrictive umask such as 0077 therefore creates import parents as 0700, not the documented 0755, which can make the imported path inaccessible to other workspace users or sandbox identities. As is already done for the imported file, explicitly chmod newly created import directories to 0755 before committing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 528c5ea: directories created by an import are fchmod'ed to 0755 through a held descriptor before the file is staged; the e2e test now runs under umask 0077.
| hash.update(chunk); | ||
| await writeImportChunk(handle, chunk, written); |
There was a problem hiding this comment.
The SHA-256 is computed from the downloaded chunks before those bytes are independently written to a temporary file, and the later installation check verifies only the inode. Because that temporary file is visible inside the user-owned workspace during the transfer, another local process can modify already-written offsets or change the file after this digest check; the altered file is then installed while the result still reports the requested SHA-256. Re-read and hash the staged inode after writing, or otherwise prevent concurrent modification until installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 528c5ea: the stream no longer drives the digest. After sync, size and SHA-256 are computed by re-reading the staged temporary file through its own descriptor, which is what gets installed. A new test rewrites staged bytes mid-transfer and expects INVALID_REQUEST with nothing installed.
| if (request.operation === 'import_file' && options.imports != null) { | ||
| const imports = options.imports; | ||
| let sessionKey: string; | ||
| try { | ||
| sessionKey = await imports.authorize(req, request); |
There was a problem hiding this comment.
Every durable import POST reauthorizes the upload before RedisWorkspaceRequests.submit can detect an existing request ID. If the initial response was lost and the upload or session record is deleted or expires before the client retries, the retry returns IMPORT_SOURCE_UNAUTHORIZED even though the matching durable request may still be queued, running, or completed and capable of mutating the workspace. Move this authorization into the new-record preparation path so a same-fingerprint retry can return the stored status without requiring the source to remain live.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 528c5ea: ownership is checked inside the durable prepare hook, which runs only for a new request ID, so a same-fingerprint retry returns the stored status even after the upload expires (covered in the durable test).
- Verify size and SHA-256 on the synced staged file, not just the stream. - Set created parent directories to 0755 regardless of umask. - Install disconnect listeners before ownership and grant minting, and never dispatch for a caller that already left. - Authorize durable imports only for new request IDs so retries of an existing request stay idempotent after the upload expires. - Gate worker advertisement behind LIBRECHAT_CODE_ALLOW_WORKSPACE_IMPORTS, since older LibreChat releases reject unknown operations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528c5eac5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| arm(); | ||
| yield next.value; |
There was a problem hiding this comment.
When writing a yielded chunk takes longer than stallTimeoutMs—for example on a slow or temporarily blocked network filesystem—the timer armed immediately before yield aborts the response while the consumer is writing, even if the source has already delivered the next or final chunk. The subsequent reader.read() then reports a source-transfer stall and rejects an otherwise valid import; arm the timer only while awaiting response headers or reader.read(), as the option contract specifies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 400c032: the timer is armed only around reader.read() (and the header wait) and cleared before each yield, so consumer writes never count. Covered by a slow-consumer test.
| async function removeCreatedDirectories(created: readonly string[]): Promise<boolean> { | ||
| for (const directory of [...created].reverse()) { | ||
| try { | ||
| await rmdir(directory); |
There was a problem hiding this comment.
When an out-of-band local process removes or renames an import-created parent and creates a new empty directory at the same path before the import fails, this pathname-only rmdir deletes the other process's directory. Transfers can leave these paths exposed for minutes, and created stores no inode identity, so rollback cannot distinguish the original directory from a replacement; retain and verify directory identity (or a held descriptor) before removing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 400c032: each created directory's dev/ino is recorded through its descriptor when its mode is set; rollback removes a directory only while it is still that inode, otherwise it reports a possibly committed mutation. Covered by a test that swaps the parent mid-import.
| const registration = await this.bridge.prepareDurableWorkspaceTool({ | ||
| ...args, tenantId: args.owner.tenantId, | ||
| }); | ||
| const request = args.prepare == null ? args.request : await args.prepare(); |
There was a problem hiding this comment.
For a new durable import, this callback mints the egress grant before the atomic queue submission, but returns only the rewritten request and discards the grant ID. If Redis fails, the queue is full, or a concurrent submission wins with the same request ID, no record uses this invocation's grant and no code revokes it, so repeated rejected submissions leave unused egress-ledger entries active until their potentially ten-minute expiry. Preserve the cleanup handle and revoke the grant unless the queue record actually adopts it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 400c032: the durable prepare hook now returns a release that revokes the grant, called when queue.submit throws or returns anything but accepted. Covered by a racing-submission test (only the adopted grant survives).
| throw new WorkspaceToolError('Workspace storage is unavailable', 'WRITE_UNAVAILABLE'); | ||
| } finally { | ||
| await handle?.close().catch(() => undefined); | ||
| if (temporaryNeedsCleanup) await unlink(temporary).catch(() => undefined); |
There was a problem hiding this comment.
When an import fails before installation and unlinking its staging file also fails—especially when the destination parent already existed—this catch suppresses the cleanup failure and rethrows the original error as though no mutation remained. The workspace can therefore retain a large, possibly already chmodded 0644 file containing upload bytes while the mutation guard is cleared; report the cleanup failure as a potentially committed mutation instead of silently ignoring it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 400c032: a staging unlink failure other than ENOENT now raises WRITE_UNAVAILABLE with mutationMayHaveCommitted, so the guard stays armed.
- Roll back a created parent only while it is still the inode the import created; otherwise report a possibly committed mutation. - Report an unremovable staging file as a possibly committed mutation. - Arm the stall timer only while waiting on the source, not during writes. - Revoke a durable import's grant when the queue does not adopt it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 400c03255e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .toLowerCase() | ||
| .replace(/\p{Default_Ignorable_Code_Point}/gu, '') | ||
| .replace(/[. ]+$/, ''); | ||
| return folded === '.git' || folded === 'git~1'; |
There was a problem hiding this comment.
On Windows with NTFS, a path such as .git::$INDEX_ALLOCATION/config passes this predicate even though ::$INDEX_ALLOCATION aliases the .git directory. installImportedFile subsequently canonicalizes that parent with realpath and installs into the resulting .git path, allowing an import to modify Git metadata despite the explicit .git prohibition. Reject NTFS alternate-data-stream syntax, particularly the $INDEX_ALLOCATION directory alias.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 393c251: .git detection now compares only the part of a segment before any :, so .git::$INDEX_ALLOCATION, .GIT:stream and the git~1 variants are refused; ordinary names containing : on POSIX stay allowed. Tests added.
Compare only the part of each segment before an NTFS stream separator, so `.git::$INDEX_ALLOCATION` and similar aliases are refused like `.git`.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The code bridge is experimental, so import_file is now on whenever a worker can perform it (writes enabled and a relay upstream configured). Operators opt out with LIBRECHAT_CODE_ALLOW_WORKSPACE_IMPORTS=false or --no-workspace-imports; any other value fails startup.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Rollout order (read first)
import_fileis on by default: the code bridge is experimental and lightly used, so a worker advertises it whenever it can actually perform it. That means workspace writes are enabled,LIBRECHAT_CODE_FILE_RELAY_UPSTREAMis configured, and the Code API offers it (EGRESS_GATEWAY_URLset, so grants can be minted). The Code API has no separate toggle; being able to mint grants is the only condition.Update LibreChat first. LibreChat releases that predate LibreChat-AI/LibreChat#17376 reject a worker's whole status/capabilities when it lists an operation they don't recognize. #17376 makes LibreChat ignore unknown worker operations (forward-compatible) and is deployed to demo before workers update. Self-hosters on an older LibreChat should update LibreChat before updating workers.
Escape hatch: if a worker must serve an older LibreChat, opt out with
LIBRECHAT_CODE_ALLOW_WORKSPACE_IMPORTS=falseorrun --no-workspace-imports. Any other value of that variable fails startup instead of silently choosing either way.The opt-out is not stored in the saved setup profile. That profile schema rejects unknown keys, so adding a field would stop older workers from starting with a profile written by a newer setup. The environment variable still applies to workers launched from a profile.
Final contract
The contract is the one agreed for the LibreChat side, with two clarifications that only pin down behavior it left open:
bytesWrittenis0whenunchanged: true, and Code API returns typed errors from/v1/workspace-tools/*before dispatch.Operation.
import_file, additive,BRIDGE_PROTOCOL_VERSIONstays1. A worker lists it incapabilities.workspaceTools.operationsand in each eligible workspace'soperationsonly when it can perform it. LibreChat sends it only when it is listed for the selected workspace.Request (LibreChat to
POST /v1/workspace-tools/requestsor/execute, same envelope and auth as the other workspace operations):{ protocolVersion: 1, operation: 'import_file', workspaceId, workspaceInstanceId?, worktree?, path, source: { storage_session_id: string, id: string }, bytes: number, sha256: string /* lowercase hex */, overwrite?: boolean /* default false */ }pathfollowswrite_file's rules and may not contain a.gitsegment or end in/or..sourceuses raw Code API storage ids (21-character nanoids).egressGrant. A request that includes it is rejected with400.Result:
{ protocolVersion: 1, operation: 'import_file', workspaceId, path, created: boolean, unchanged?: boolean, bytesWritten: number, sha256: string }created: true,bytesWritten === bytes.created: false,unchanged: true,bytesWritten: 0. Nothing is downloaded or written.overwrite: truereplacing different bytes:created: false,bytesWritten === bytes.Errors (
{ error, code }):INVALID_WORKSPACE_TOOL_REQUESTegressGrantIMPORT_SOURCE_UNAUTHORIZEDsourceIMPORT_UNAVAILABLEWRITE_LIMIT_EXCEEDEDbytesis over the Code API or worker limit, or the target filesystem lacks free spaceEDIT_CONFLICToverwrite !== true, or it changed during the importWRITE_DISABLEDINVALID_PATH.gitpath, symlinked parent, or traversalINVALID_REQUESTbytesorsha256WRITE_UNAVAILABLECode API returns
INVALID_WORKSPACE_TOOL_REQUEST,IMPORT_SOURCE_UNAUTHORIZED,IMPORT_UNAVAILABLEand its own size check directly, before dispatch, including on the durable submit. The worker returns the other codes, plusWRITE_LIMIT_EXCEEDEDfor its own limit or free space. On a durable request, the worker's codes appear in the request statuserror.code.Why
When a LibreChat chat runs on an attached personal-machine workspace (a BYOM bridge to a paired
@librechat/codeworker), files the user uploads in that chat never reach the workspace. Only programmatic Bash stages inputs, and only for the length of one run, in$LIBRECHAT_CODE_DATA_DIR.write_filetakes only UTF-8 text up to 1 MiB, so zip files, images and PDFs cannot get there at all. Users expect an upload to appear in the workspace, binaries included.How it works
LibreChat sends
import_filewith the upload's storagesource, its size, and its SHA-256.Code API checks that the caller owns the object. It applies the same rule as
GET /download/:session_id/:fileIdfor a user-kind object:session:<storage_session_id>key must equal the caller's own storage key;upload:<key><session><id>record must exist for that key, session and object.An expired session cache is refused. The caller's claim is never trusted. For durable requests, ownership is checked only when a new request ID is accepted, so a retry of an existing request returns its stored status even if the upload has since expired.
Code API asks the egress gateway (
POST /internal/egress-grants) for a grant that covers that one object only:input_fileslist holds just that object;max_upload_bytes: 0andmax_output_files: 0, so nothing can be uploaded;max_requests: 2, one download plus one retry;The request relayed to the worker carries the grant, and
sourceis replaced by the gateway's opaque session and object handles. A synchronous request revokes its grant when it finishes. A durable request lets its grant expire. Durable requests fingerprint the caller's request, so a retry with the same request ID returns the existing record and does not mint a second grant.The worker downloads the object through
LIBRECHAT_CODE_FILE_RELAY_UPSTREAM, the same egress route programmatic inputs already use. It holds no storage credentials.Worker behavior
write_file, with workspace and worktree confinement. It also refuses any.gitsegment, including aliases created by case folding, Unicode-ignorable characters, trailing dots or spaces, and theGIT~1short name. It refuses a parent directory that is a symlink, a final path segment that is a symlink, and any path that resolves outside the root.unchanged: true;overwrite: true: returnsEDIT_CONFLICT.0755.bytesis a hard cap while streaming.0644and is synced; size and SHA-256 are then verified by re-reading the synced temporary file through its own descriptor (what gets installed, not just what arrived), and it is installed atomically.0755explicitly, so a restrictive worker umask does not change them.write_fileuses. Bridge mutations of one root are serialized, so only an out-of-band local writer could race that last step.Security model
write_fileconfinement plus the.git, symlink and canonical-root checks above, enforced again at commit time.CODEAPI_BRIDGE_IMPORT_FILE_MAX_BYTES, default 100 MiB, checked before ownership or dispatch.LIBRECHAT_CODE_IMPORT_FILE_MAX_BYTES/--import-file-max-bytes, default 100 MiB.import_fileis a file write. It arms the same durable mutation guard aswrite_file. It is never advertised for a workspace without writes enabled, and such a workspace refuses it withWRITE_DISABLED. LibreChat treating the upload as consent is decided on its side; the worker and Code API policies still apply.Compatibility
Additive: protocol version 1 is unchanged, and no existing request or result shape changes.
Code API: lists
import_fileinsupportedWorkspaceToolOperationsonly whenEGRESS_GATEWAY_URLis configured, because otherwise it cannot mint the grant.Worker: advertises
import_fileonly when all four hold:LIBRECHAT_CODE_ALLOW_WORKSPACE_IMPORTS=false/--no-workspace-imports; on by default, see Rollout order);LIBRECHAT_CODE_FILE_RELAY_UPSTREAMis set;An older Code API therefore never sees it, and a worker without a relay never offers it.
LibreChat: sends
import_fileonly when it is advertised. Older workers and Code APIs keep their current behavior.Parent directories: as with
write_file, they are created only beneath a descriptor-held root (project roots). A legacy pathname-only root reports a missing parent instead.Changes
packages/code/src/protocol.ts:isWorkspaceImportFilePath,isBridgeWorkspaceImportFileRequest(caller form, never carries a grant) andisRelayedWorkspaceImportFileRequest(worker form, must carry one);isWorkspaceMutationOperation, now used by both the worker and the store.packages/code/src/workspace.ts: the confined, streaming, atomic import, andimport_fileadvertisement on writable roots that have an importer.packages/code/src/workspace-import.ts:HttpWorkspaceFileImporter, a bounded, stall-timed streaming download through the relay upstream that also reports free space.relay.ts: now holds the grant header and object URL helpers that programmatic inputs and imports share.cli.ts,workspace-instances.ts,linked-worktrees.ts: wiring, so conversation worktrees and linked-worktree lanes also support imports.service/src/workspace-tools/import.ts: the ownership check, grant claims and the import service.router.ts: validation, limits, authorization, grant minting and revocation, and the import execution budget (min(JOB_TIMEOUT, 5 min)). Disconnect listeners are installed before ownership and grant minting, and a caller that leaves meanwhile never reaches dispatch; its grant is revoked.requests.ts: an optionalpreparehook, so a durable request mints its grant once.bridge/router.tsandbridge/enabled.ts: the capability offer.config.ts: the size limit.packages/codeREADME,docs/remote-bridge, OpenAPI, and docker-compose.Tests
packages/code/src/workspace-import.test.ts(a realLocalWorkspaceToolsandHttpWorkspaceFileImporteragainst a local egress stub, plus a realBridgeWorker) covers:.gitaliases, bad hashes, extra keys and the caller-grant split;0755parents and a0644file;0644);.gitpath and missing grant;service/src/workspace-tools/import.test.ts(ioredis-mock, the real session ownership and session-key code, and realprepareSandboxEgressgrant sealing) covers:IMPORT_UNAVAILABLEwhen no grants can be minted;RedisWorkspaceRequests: one grant across idempotent retries, and a conflict before minting.service/src/bridge/router.test.ts: registration offersimport_fileonly when grants can be minted.tests/compose-bridge-config.cjs: the API size-limit pass-through.