✍️ feat: Add Opt-In BYOM Workspace Mutations - #95
Conversation
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: b040b87ada
ℹ️ 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".
| await atomicWriteConfinedFile(root, request.path, updated, { | ||
| dev: openedStat.dev, | ||
| ino: openedStat.ino, | ||
| }); |
There was a problem hiding this comment.
When another process rewrites the file in place while edit_file is running, the inode remains unchanged, so passing only dev and ino as the expected state lets the later checks succeed and the edit silently overwrite those concurrent changes. This contradicts the stated conflict behavior and can corrupt an actively modified workspace; verify the content or change-sensitive metadata immediately before committing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd40b09. Exact edits now re-open the target and compare bounded current bytes with the original bytes immediately before commit, in addition to inode and canonical-path checks. Covered by the workspace mutation tests and the 63-test focused workspace/protocol/worker run.
| ) { | ||
| throw new WorkspaceToolError('Invalid workspace path', 'INVALID_PATH'); | ||
| } | ||
| const original = await opened.readFile(); |
There was a problem hiding this comment.
When another process grows the opened file after openedStat.size is checked, opened.readFile() buffers the new contents without the 1 MiB bound enforced by the preceding snapshot. A concurrently appended file can therefore make an edit consume arbitrarily large memory before the later write-size check rejects it; read at most the configured limit plus one byte, as the confined read path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd40b09. Edit reads now use a fixed 1 MiB plus one-byte buffer and return WRITE_LIMIT_EXCEEDED when the bound is crossed. Added a regression test for oversized edit sources.
| handle = await open( | ||
| temporary, | ||
| constants.O_WRONLY | | ||
| constants.O_CREAT | | ||
| constants.O_EXCL | | ||
| constants.O_NOFOLLOW, | ||
| existing == null ? 0o600 : Number(existing.mode) & 0o777, | ||
| ); |
There was a problem hiding this comment.
When replacing an existing file whose mode includes permissions masked by the worker's umask—for example mode 0664 under the common 0022 umask—passing that mode to open() creates the temporary file as 0644. The subsequent rename therefore silently changes permissions on ordinary writes and edits; apply the recorded mode with chmod/fchmod after creation so it is not filtered by the process umask.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd40b09. Existing modes are now restored with FileHandle.chmod after temp creation so umask cannot silently narrow them. Added a 0664-under-umask regression test.
| 'EDIT_CONFLICT', | ||
| ); | ||
| } | ||
| await rename(temporary, candidate); |
There was a problem hiding this comment.
When a workspace assignment is cancelled or reaches its deadline after the initial signal check but while the temporary file is being written or synced, this unconditional rename still commits the mutation. The worker then observes the aborted signal and reports a rejected settlement, leaving the caller believing the operation failed even though the workspace changed; propagate the signal into the write path and check it immediately before the rename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd40b09. The abort signal is propagated through write and edit paths and checked immediately before atomic rename, after source verification and fsync. Existing aborted-execution coverage remains green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd40b09d46
ℹ️ 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".
| await verifyEditSource(root, candidate, expected); | ||
| } | ||
| throwIfAborted(signal); | ||
| await rename(temporary, candidate); |
There was a problem hiding this comment.
When the requested parent is an in-workspace directory symlink, another process can repoint that symlink outside the workspace after realpath(parent) is checked but before this rename. The temporary source remains under canonicalParent, while candidate is resolved again through the changed symlink, allowing a write outside the registered root; reject symlinked parents or perform the validation and rename relative to a held directory descriptor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ec2ab4. Mutations now reject any parent whose canonical path differs from its lexical path, capture the non-symlink directory inode, and revalidate that same parent identity immediately before rename. Added coverage for an in-workspace directory symlink target.
| const first = text.indexOf(request.oldText); | ||
| if ( | ||
| first < 0 || | ||
| text.indexOf(request.oldText, first + request.oldText.length) >= 0 |
There was a problem hiding this comment.
When oldText overlaps with itself, advancing the second search by its full length misses another valid occurrence—for example, oldText: "aa" in "aaa" matches at offsets 0 and 1 but is accepted as unique. The edit then silently chooses the first of multiple possible locations instead of returning EDIT_CONFLICT; search for another occurrence beginning at first + 1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ec2ab4. Uniqueness now searches again from first + 1, so overlapping matches such as aa in aaa return EDIT_CONFLICT. Added a regression assertion.
| if (existing != null) { | ||
| await handle.chmod(Number(existing.mode) & 0o777); | ||
| } |
There was a problem hiding this comment.
When an existing file's group differs from the worker's primary group, the replacement temporary inode is created with the worker's default ownership, and chmod restores only the mode bits. After the rename, group-based collaborators can therefore lose access even though the permission-preservation check reports success; copy the original UID/GID to the temporary handle before installing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ec2ab4. POSIX replacements now restore the original UID and GID on the temporary descriptor before restoring mode and committing. Added an alternate-group ownership regression test with safe platform skips.
| workspaces: supportsEntireProtocol | ||
| ? desired.workspaces | ||
| : desired.workspaces.map(({ operations: _, ...workspace }) => | ||
| workspace, |
There was a problem hiding this comment.
When the Code API advertises a supported subset that includes a mutator—for example write_file but not edit_file—and the executor has mixed read-only and writable workspaces, this branch removes every per-workspace operation restriction. The resulting registration advertises write_file for read-only workspaces, so the service dispatches assignments that the worker subsequently rejects instead of preventing the unsupported tool call; intersect each workspace's operations with the promoted global set rather than dropping the field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ec2ab4. Partial mutation negotiation now intersects each workspace operation set with the promoted global set and omits workspaces with no supported operation. Legacy read/search/list promotion still strips the new descriptor field. Added mixed read-only/writable rolling-upgrade coverage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec2ab4b3d
ℹ️ 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 (existing != null) { | ||
| if (process.platform !== 'win32') { | ||
| await handle.chown(Number(existing.uid), Number(existing.gid)); | ||
| } | ||
| await handle.chmod(Number(existing.mode) & 0o777); |
There was a problem hiding this comment.
When replacing a group- or world-accessible file in a shared directory, restoring its ownership and mode before writing makes the temporary inode readable—and potentially writable—by collaborators while its contents are incomplete. A collaborator that discovers the temporary name can observe partial data or retain a writable descriptor through the rename and corrupt the installed file; keep the temporary file owner-only until its content is complete, then apply the preserved metadata immediately before installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b1a47c. The temporary inode now stays 0600 through content write and fsync; preserved ownership/mode are applied only after target and parent revalidation, then synced immediately before close/install. The permission and ownership tests pass.
| canonicalParent = await realpath(parent); | ||
| parentIdentity = await lstat(parent); | ||
| if ( | ||
| canonicalParent !== parent || |
There was a problem hiding this comment.
On case-insensitive macOS or Windows filesystems, a valid request can spell an existing directory with different casing from the stored name, while realpath(parent) returns the canonical spelling. The strict string comparison then reports INVALID_PATH, even though reads of the same path and the filesystem itself accept it; detect symlink components without requiring the requested spelling to equal the canonical path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b1a47c. Parent confinement now walks each requested directory component with lstat to reject symlinks without requiring requested casing to equal realpath casing. Installation targets the validated canonical parent. A conditional case-insensitive-filesystem regression test passes on macOS.
| return request.operation === 'write_file' | ||
| ? writeWorkspaceFile(root, request, signal) | ||
| : editWorkspaceFile(root, request, signal); |
There was a problem hiding this comment.
When write_file or edit_file succeeds locally but delivery of the fulfilled settlement is rejected or remains ambiguous, settleWithRetry quarantines only assignments with a runtimeSessionId; workspace-tool assignments have none, so the run loop reconnects and continues even though the caller can receive a failure for an already-applied mutation. Treat successful mutation assignments as stateful and stop or quarantine the worker on ambiguous settlement so subsequent operations cannot proceed from an unacknowledged workspace state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b1a47c. Successful write_file/edit_file execution is now tracked as committed state. Definite settlement rejection, ambiguous delivery, shutdown before settlement, or a post-commit cancellation/deadline race raises BridgeWorkspaceQuarantinedError so run() exits. Three transition tests cover these paths.
| } catch (error) { | ||
| if (error instanceof WorkspaceToolError) throw error; | ||
| throw new WorkspaceToolError('Workspace write failed', 'INVALID_PATH'); |
There was a problem hiding this comment.
When a valid write fails because of an operational filesystem error such as ENOSPC, an I/O failure during sync, or a transient permission/ownership failure, this catch converts every such error to INVALID_PATH. The HTTP layer consequently returns a non-retryable 422 that tells callers the path is bad, masking worker or storage failures; introduce an unavailable/write-failure error classification and reserve INVALID_PATH for actual confinement or path-validation failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9b1a47c. Operational filesystem failures now surface as WRITE_UNAVAILABLE and map to HTTP 503, while concrete confinement/path-validation failures remain INVALID_PATH. Package and router regression tests cover both classification and HTTP mapping.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b1a47c757
ℹ️ 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".
| await handle.chown(Number(existing.uid), Number(existing.gid)); | ||
| } | ||
| await handle.chmod(Number(existing.mode) & 0o777); | ||
| await handle.sync(); |
There was a problem hiding this comment.
When another process updates the target after verifyEditSource returns, these asynchronous metadata operations leave a window before rename, allowing an exact edit to report success while silently overwriting the concurrent bytes instead of returning EDIT_CONFLICT. I reproduced this at the exact reviewed head by rewriting the target as soon as the temporary file's mode was restored; the edit succeeded and replaced that rewrite. This is fresh evidence at the current head because metadata restoration was moved after the final source comparison; restore metadata before that comparison or revalidate the source and target identity again after these awaits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bbceef9. Existing ownership/mode are now restored and synced before the final target-content, target-identity, and parent-identity revalidation. The only remaining step before rename is a synchronous abort check. Added a deterministic regression that rewrites the target from the temporary FileHandle.chmod seam and verifies EDIT_CONFLICT preserves the concurrent update. The full @librechat/code suite passes (181 pass, 1 platform skip), and the live Code API/outbound-worker write-edit-read path passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbceef9565
ℹ️ 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".
| const [currentParent, currentParentIdentity] = await Promise.all([ | ||
| realpath(parent), | ||
| verifyDirectoryPathHasNoSymlinks(root, parent), | ||
| ]); |
There was a problem hiding this comment.
When another process rewrites the target in place while these awaited parent checks run, the exact byte comparison at line 467 has already completed, so the subsequent rename reports success and overwrites the concurrent update instead of returning EDIT_CONFLICT. Fresh evidence at the exact bbceef95655fa16e8a700a40a75b31957fb613a1 head is that Promise.all still performs asynchronous filesystem work after the final content check; make the byte check the last operation before rename, without awaiting handle closure or other work between them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fcfc4e9. Parent identity is revalidated first, then the edit source is reopened and its exact bytes are checked immediately before submitting the atomic rename; the source descriptor is closed only afterward. Covered by the edit revalidation and full package suite (186 passed, 1 platform skip), plus live write/edit/read verification.
| } catch (error) { | ||
| if (error instanceof WorkspaceToolError) throw error; | ||
| throw new WorkspaceToolError('Invalid workspace path', 'INVALID_PATH'); |
There was a problem hiding this comment.
When a valid edit_file request encounters an operational error during its initial open, stat, or bounded read—for example EIO or EMFILE—this blanket catch reports INVALID_PATH, which the HTTP layer turns into a non-retryable 422 instead of the 503 used for WRITE_UNAVAILABLE. Fresh evidence at the exact reviewed head is that the new write-error classifier is applied only inside the atomic replacement path, while this pre-write edit path still converts every non-WorkspaceToolError; classify only actual confinement/path failures as INVALID_PATH and surface operational failures as unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fcfc4e9. Initial edit open/stat/read operational failures now use the same bounded write-path classifier: missing/path-shape failures remain INVALID_PATH while storage failures return WRITE_UNAVAILABLE. The locked-directory test now covers both write_file and edit_file; the full package suite passes (186 passed, 1 platform skip).
| if (ambiguousWorkspaceMutationError != null) { | ||
| throw await this.quarantineWorkspace( | ||
| undefined, | ||
| 'Worker stopped after a workspace mutation completed without a fulfilled settlement', | ||
| ambiguousWorkspaceMutationError, |
There was a problem hiding this comment.
When settlement of an already-applied mutation is ambiguous and the CLI is managed by a supervisor with automatic restart, this call passes no runtime session, so quarantineWorkspace(undefined, ...) merely constructs a fatal error and records no local or server-side fence. A restarted process gets a new incarnation and can resume operations on the unacknowledged directory once the old assignment expires, defeating the quarantine intended to prevent subsequent work from observing ambiguous state. Fresh evidence at the exact reviewed head is the undefined quarantine target here together with the helper's immediate no-op return for missing session IDs; persist a workspace/worker quarantine that survives process restart until an operator explicitly clears it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fcfc4e9 with the broader restart invariant addressed. Writable workers now require durable quarantine storage; a deployment/worker/workspace marker is armed before each mutation and cleared only after Code API accepts settlement. Ambiguous outcomes keep/update it, restart registration fails closed, malformed/unreadable state fails closed, and recovery requires the explicit clear-workspace-quarantine command. Tests cover ordering, ambiguity, restart refusal, arm failure, persistence/mode, and explicit clearing; live Code API verification also proved restart refusal and recovery.
There was a problem hiding this comment.
Follow-up hardening in 81a4253: after the invariant pass, an unknown/custom mutation-executor failure is now treated as an ambiguous outcome, so its pre-armed durable marker is retained and no rejection settlement or clear can make the worker reusable. A focused transition test proves arm → execute → quarantine with no settle/clear.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81a4253c7e
ℹ️ 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".
| } finally { | ||
| await file.close(); | ||
| } | ||
| await rename(temporaryPath, path); |
There was a problem hiding this comment.
When the host crashes or loses power after this rename but before the parent directory entry is persisted, the workspace mutation may survive while the newly armed quarantine marker disappears. A restarted worker then passes assertAvailable() and resumes against an ambiguously mutated workspace, defeating the durable restart fence; fsync the containing directory after installing the marker before allowing mutation execution to begin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c80768b. Marker persistence now fsyncs the installed file and then opens/fsyncs its parent directory before arm() returns, so mutation execution cannot begin before the directory entry is durable. The storage test verifies both sync points and 0600 mode.
| try { | ||
| await this.options.workspaceMutationQuarantine?.assertAvailable(); |
There was a problem hiding this comment.
When a mutation remains in execution until the registration heartbeat is due—for example, an assignment leased near the 30-second half-TTL followed by a slow filesystem write—this heartbeat calls register(), and the CLI-backed assertAvailable() finds the marker that the same execution just armed. maintainRegistration() treats the resulting WORKER_QUARANTINED error as terminal and aborts the active execution, potentially turning an otherwise valid mutation into a persistent quarantine; distinguish startup/restart validation from heartbeats belonging to the currently armed mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c80768b. Startup/public registration still checks durable state, while the private heartbeat path skips only the marker owned by this worker while its mutation guard is active. The guard is set before asynchronous arming and cleared only after durable marker removal. A transition test forces a heartbeat during arm/execution and verifies registration continues without another availability check.
| 'EDIT_CONFLICT', | ||
| ); | ||
| } | ||
| await rename(temporary, installTarget); |
There was a problem hiding this comment.
When cancellation or the deadline fires while an exact edit is preparing its temporary file, the expected != null branch reaches this rename without the throwIfAborted(signal) used by the ordinary write branch, so the edit can still be installed after execution was aborted. Fresh evidence at exact head 81a4253c7e5dd68b7ff6515c7fd80c83ad09f29b is that the later fcfc4e9 refactor moved the edit rename into commitVerifiedEdit without passing the signal; recheck it immediately before this rename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c80768b. commitVerifiedEdit again receives the execution signal and checks it synchronously after the final exact-byte comparison and immediately before rename. The new failure-injection test aborts during temporary-file preparation and proves the original target remains unchanged with EXECUTION_ABORTED.
| payload = await this.options.workspaceTools.execute( | ||
| workspaceRequest, | ||
| executionController.signal, | ||
| ); | ||
| workspaceMutationApplied = isMutation; |
There was a problem hiding this comment.
When a custom workspace executor applies a mutation but returns a malformed result, this code marks the mutation as applied and later submits a fulfilled settlement without validating payload. The settlement endpoint accepts it, causing the worker to clear its durable marker, while dispatchWorkspaceTool() validates the result only after dispatch has committed and then reports RESULT_INVALID to the caller; validate the result against the request before accepting the fulfilled settlement so an invalid post-mutation response remains quarantined.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c80768b. Mutation results are validated against their originating request before fulfilled settlement. An invalid custom result is treated as an ambiguous post-mutation outcome: no settlement or clear occurs and durable quarantine remains. The transition test proves arm → execute → quarantine only.
| 'librechat', | ||
| 'code', | ||
| 'quarantines', | ||
| workspaceStorageName(options.codeApiUrl.replace(/\/+$/, '')), |
There was a problem hiding this comment.
When an operator restarts a worker using the same explicit --worker-dir and endpoint but changes the URL to an equivalent spelling, such as https://code.example/v1 to https://CODE.EXAMPLE:443/v1, this raw-string hash selects a new empty quarantine path. The worker therefore bypasses an existing ambiguous-mutation marker and resumes against the same directory; canonicalize the URL's scheme, host, default port, and path before deriving the deployment key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c80768b. Quarantine keys now canonicalize the deployment URL through URL parsing, normalizing scheme/host casing, default ports, and trailing path slashes before hashing. Tests prove equivalent URL spellings resolve to the same marker while workers/workspaces remain isolated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c80768be74
ℹ️ 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".
| } | ||
| await rename(temporaryPath, path); | ||
| await chmod(path, 0o600); | ||
| await syncParentDirectory(path); |
There was a problem hiding this comment.
On a worker's first mutation, mkdir(..., { recursive: true }) may create several quarantine directories, but this syncs only the deepest directory. A crash after arming and subsequently mutating the workspace can therefore preserve the workspace change while losing one of the new ancestor entries—and thus the marker—even though arm() returned successfully. Sync each newly created directory's parent, or provision and sync the hierarchy before treating the guard as durable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 690c42f. Quarantine directory provisioning now records every absent ancestor, creates the hierarchy, and fsyncs each new directory parent from shallowest to deepest before installing and syncing the marker. The storage transition test exercises a newly created nested hierarchy and verifies all sync boundaries.
| 'quarantines', | ||
| workspaceStorageName(canonicalDeploymentUrl(options.codeApiUrl)), | ||
| workspaceStorageName(options.workerId), | ||
| `${workspaceStorageName(options.workspaceId)}.json`, |
There was a problem hiding this comment.
When a quarantined worker is restarted with the same explicit --worker-dir but a changed --workspace-id, this workspace-ID-only component selects a clean marker path and allows mutations against the same unacknowledged directory state. This is fresh evidence at exact head c80768be74d8f556d7cc52bef078d12af89c9189: URL aliases are now canonicalized, but the independently configurable workspace ID can still alias one physical root. Include a stable identity for the canonical workspace directory in the quarantine key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 690c42f. The CLI now realpaths the selected workspace and derives the marker from deployment + worker + canonical physical root, not the logical workspace ID. The CLI integration test proves changing the ID cannot bypass the marker, and live Code API verification repeated that restart/clear/reconnect flow.
| export async function clearWorkspaceMutationQuarantine( | ||
| path: string, | ||
| ): Promise<void> { | ||
| await rm(path, { force: true }); |
There was a problem hiding this comment.
When a fulfilled settlement clears the marker and the host crashes shortly afterward, rm() can return before the containing directory records the deletion durably, so the marker may reappear on restart and permanently block an otherwise clean worker until another manual clear. Fsync the containing directory after removal, just as the creation path now does after installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 690c42f. Explicit marker removal now checks existence, removes it, and fsyncs the containing directory before returning. The storage test verifies the additional removal sync and idempotent absent-marker behavior remains intact.
| ? [] | ||
| : [{ ...workspace, operations: workspaceOperations }]; | ||
| }) | ||
| : desired.workspaces.map(({ operations: _, ...workspace }) => workspace); |
There was a problem hiding this comment.
When the API supports only a partial read-only subset and a custom executor restricts reads per workspace—for example, globally supporting read_file and list_files while one workspace permits only read_file—this branch strips that workspace's restriction and advertises list_files for it. The service can then dispatch the call, only for executeAndSettle() to reject it against the executor's original capabilities. Fresh evidence at the exact head is that the earlier partial-mutation fix preserves restrictions only when the promoted subset contains a mutator; intersect restrictions for every partial subset instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 690c42f. Capability promotion now intersects every per-workspace restriction for every supported subset, including read-only subsets. A focused rolling-compatibility test verifies read_file-only and listable workspaces remain distinct after promotion.
| } | ||
| if ( | ||
| workspaceMutationApplied || | ||
| (workspaceMutationArmed && !(error instanceof WorkspaceToolError)) |
There was a problem hiding this comment.
When a custom mutation executor applies its filesystem change and then throws a WorkspaceToolError—for example, WRITE_UNAVAILABLE after a post-commit durability step—this exemption treats the outcome as clean, submits a rejected settlement, and clears the durable marker. The caller is then told the mutation failed even though the workspace changed, and a restart is not fenced. Fresh evidence at exact head c80768be74d8f556d7cc52bef078d12af89c9189 is that only untyped executor failures retain quarantine; a WorkspaceToolError carries no contract proving that no mutation was applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 690c42f. WorkspaceToolError alone is no longer treated as proof of a clean mutation failure. The executor must explicitly declare mutationFailuresAreAtomic; the built-in atomic executor does so. Tests prove typed errors from an untrusted custom executor remain quarantined, while a declared atomic rejection is settled and cleared.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 690c42fda6
ℹ️ 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".
| return { created: false }; | ||
| } | ||
| throwIfAborted(signal); | ||
| await rename(temporary, installTarget); |
There was a problem hiding this comment.
When the host crashes or loses power after this rename and the fulfilled settlement is accepted, the temporary file's contents are durable but the containing directory entry is not. The replacement or newly created file can therefore disappear or revert after restart even though Code API has acknowledged success and the durable quarantine marker has been cleared. Open and fsync the containing directory after both this rename and the exact-edit rename before reporting the mutation as complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16226e6. Both write and exact-edit replacements now fsync the containing workspace directory after rename. A post-rename sync/open/close failure is explicitly classified as mutationMayHaveCommitted, so even the built-in atomic executor retains durable quarantine. Tests cover both write/edit directory sync and the post-commit failure path; @librechat/code passes 199 tests with one platform skip.
| workspaces: workspaceTools.workspaces.map(({ operations: _, ...workspace }) => | ||
| workspace, |
There was a problem hiding this comment.
When the Code API omits supportedWorkspaceToolOperations, this compatibility registration still removes every per-workspace restriction. For example, a custom executor that globally supports read_file and search_text but permits only read_file in one workspace advertises search_text there to the legacy service, which can dispatch it only for executeAndSettle() to reject it. At this exact head, the partial-promotion path preserves restrictions, but this no-negotiation fallback remains unchanged; omit workspaces that cannot safely be represented rather than widening their operation set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16226e6. Legacy registration now omits workspaces whose operation restrictions cannot be represented without widening, and execution is fenced against the capabilities accepted by the active registration. The regression test verifies both the reduced registration and rejection of an attempted dispatch to the omitted workspace; @librechat/code passes 199 tests with one platform skip.
9466708 to
16226e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16226e6b1f
ℹ️ 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".
| throwIfAborted(signal); | ||
| await rename(temporary, installTarget); |
There was a problem hiding this comment.
At the exact reviewed head, when another process can write the target directory, it can observe the temporary name, unlink it, and create a replacement at that path before this path-based rename; mode 0600 protects the staged inode's contents but does not prevent unlinking through a writable directory. Both writes and exact edits can consequently install attacker-controlled or stale content while reporting the requested byte count and success, so stage in a directory inaccessible to collaborators or otherwise ensure the renamed path still denotes the held inode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1847781. The staged inode remains open through installation on POSIX, its identity is captured before rename, the containing directory is synced, and the installed path is reopened and checked for the same inode and exact requested bytes before success. Any mismatch is classified as possibly committed and retains quarantine. A fault-injection test replaces the staging name and verifies rejection/quarantine classification. @librechat/code passes 201 tests with one platform skip.
| } finally { | ||
| await file.close(); | ||
| } | ||
| await rename(temporaryPath, path); |
There was a problem hiding this comment.
During an overlapping supervisor restart on POSIX, the new incarnation can pass assertAvailable() before the old incarnation finishes an ambiguous mutation and writes its quarantine marker; when the new incarnation subsequently arms its own mutation, this rename silently replaces that existing marker and execution proceeds against the unacknowledged workspace state. The same unowned marker can later be overwritten or cleared by either incarnation, so arming and clearing need exclusive, ownership-aware compare-and-set semantics rather than unconditional replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1847781. Guard creation now uses exclusive creation at the stable marker path, records the arming incarnation as owner, and permits automatic clear/quarantine retention only for that owner. A concurrent incarnation can neither replace nor clear the active marker; incomplete creation remains fail-closed. Tests cover competing arm and wrong-owner clear. @librechat/code passes 201 tests with one platform skip.
|
Codex Review: Didn't find any major issues. Bravo. 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". |
2 participants
Summary
I added opt-in, worker-local file mutations for BYOM workspaces while keeping every workspace read-only by default.
write_fileand exact-matchedit_fileprotocol operations with strict request and result validation.--allow-workspace-writesorLIBRECHAT_CODE_ALLOW_WORKSPACE_WRITES=truebefore a worker advertises or executes mutations.Change Type
Testing
npm testinpackages/code: 171 passed and one platform-specific test skipped.bun test src/bridge/router.test.ts src/bridge/workspace-store.test.ts src/workspace-tools/router.test.tsinservice: 19 passed.bun run buildinservice; it completed with the existing Rollup and TypeScript warnings.Test Configuration:
remote-bridgebackend in local authentication modeChecklist