🧭 fix: Report Missing Workspace Paths as NOT_FOUND and Create Missing Parents on Write - #301
Conversation
… Parents on Write
A missing file, directory or search scope no longer reports INVALID_PATH. When
every existing ancestor resolves to a directory inside the workspace, the
worker returns NOT_FOUND ("Workspace path does not exist"); escapes, symlinks,
files used as directories and other path-safety failures keep INVALID_PATH.
write_file creates missing parent directories beneath the workspace or lane
root, one verified level at a time, never through a symlink or an existing
file, and removes the directories it created if the write then fails.
Code API advertises the added code in registration and maps it to HTTP 422
like INVALID_PATH. A worker reports NOT_FOUND only to a Code API that
advertised it, and resends a refused settlement with INVALID_PATH, so older
Code API replicas keep accepting settlements.
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: 098bf0679c
ℹ️ 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 mkdir(current, 0o777); | ||
| created.push(current); | ||
| createdHere = true; |
There was a problem hiding this comment.
When a write creates one or more missing parent directories, only the final containing directory is synced later by confirmInstalledMutation; the directory entries created here are never synced in their respective parents. After an acknowledged nested write, a crash can therefore lose an intermediate directory—and consequently the new file—even though the existing write path promises durable confirmation. Sync each parent after creating its child before returning success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e82c9e. Each directory this write creates is now synced into its parent (syncWorkspaceDirectory(dirname(created))) right after mkdir, before the file is installed. confirmInstalledMutation still syncs the final parent, so every new entry in the chain is durable before success is acknowledged.
| async function removeCreatedDirectories(created: readonly string[]): Promise<void> { | ||
| for (let index = created.length - 1; index >= 0; index -= 1) { | ||
| await rmdir(created[index]).catch(() => undefined); |
There was a problem hiding this comment.
If cleanup fails because of an I/O or permission error, or because another process makes a created directory nonempty, swallowing the rmdir failure leaves directories created by this rejected write in the workspace. The original error still has mutationMayHaveCommitted === false, while LocalWorkspaceTools advertises mutationFailuresAreAtomic, so the worker will not quarantine the now-mutated workspace. A cleanup failure must instead be surfaced as mutation uncertainty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e82c9e. Cleanup now goes through withCreatedDirectoriesRemoved. A directory that is already gone (ENOENT) or that another writer has filled (ENOTEMPTY/EEXIST) no longer belongs to this write, so the rejection stays atomic. Any other rmdir failure re-raises the original error with mutationMayHaveCommitted: true, so the worker quarantines instead of reporting an atomic failure. A new test aborts the write after the parents exist and checks that they are removed and that the rejection stays atomic.
| let entry: Awaited<ReturnType<typeof lstat>> | undefined; | ||
| try { | ||
| entry = await lstat(current); | ||
| } catch (error) { | ||
| if (!isMissingEntry(error)) throw error; | ||
| } |
There was a problem hiding this comment.
For a workspace without a descriptor-bound identity, an existing directory is accepted from this pathname-based lstat but no handle to it is retained. If a background process renames that directory and replaces it with a symlink before the next loop iteration, the later mkdir(current, ...) follows the replacement and can create the next directory outside the workspace; the post-creation realpath check only detects the escape after the external mutation has occurred. Traverse and create relative to held, no-follow directory descriptors rather than reusing the verified pathname.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e82c9e. Parent creation now runs only when the root is held by descriptor (holdsWorkspaceRoot()). That covers selected project roots, linked-worktree lanes and conversation worktrees, which all carry a captured identity. On those roots, mkdir and lstat go through mkdirat/openat from the held root fd, and each parent descriptor is checked for root ancestry before use, so a swapped symlink cannot redirect creation outside the root.
A pathname-only root (legacy --directory without identity) creates nothing. Its write reports the missing parent as NOT_FOUND, as before this PR except for the clearer code. Tests cover both cases: deep creation on a held root, and no creation on a pathname-only root.
…nup Failure Parent directories are created only under a held root descriptor, where mkdirat cannot follow a swapped path out of the root. A pathname-only root creates nothing and reports the missing parent as NOT_FOUND. Each created directory is synced into its parent before the write is acknowledged, and a rejected write that cannot remove a directory it created now reports a possible mutation instead of an atomic failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e82c9e83b
ℹ️ 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".
| let removed = true; | ||
| for (let index = created.length - 1; index >= 0; index -= 1) { | ||
| try { | ||
| await rmdir(created[index]); |
There was a problem hiding this comment.
When a write is rejected after creating parent directories, each successful rmdir changes an entry in the containing directory, but that parent is never synced. A crash after the rejection can therefore restore one of the already-fsynced created directories even though LocalWorkspaceTools advertises mutationFailuresAreAtomic, so the worker will not quarantine the mutated workspace. Sync each containing directory after removal and report any sync failure as mutation uncertainty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0f6d78. withCreatedDirectoriesRemoved now syncs the containing directory after each successful rmdir. A failure in the removal or in its sync leaves the rejection uncertain (mutationMayHaveCommitted: true), so the worker quarantines instead of reporting an atomic failure.
| await rmdir(created[index]); | ||
| } catch (cleanupError) { | ||
| const code = (cleanupError as NodeJS.ErrnoException).code; | ||
| if (code !== 'ENOENT' && code !== 'ENOTEMPTY' && code !== 'EEXIST') { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier cleanup thread is that ENOTEMPTY need not mean another writer filled the directory: if atomicWriteConfinedFile fails after staging, its finally block suppresses failure to unlink its own .librechat-code-*.tmp file. That leaked temporary then makes this rmdir return ENOTEMPTY, which is ignored, leaving both the temporary and created directories while the error still says no mutation may have committed. Treat this result as benign only after establishing that no write-owned temporary remains; otherwise mark the failure uncertain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0f6d78. ENOTEMPTY/EEXIST are no longer treated as benign. The only cleanup result accepted as clean is a directory that is already gone (ENOENT). Any directory left in place, including one holding a leaked .librechat-code-*.tmp, marks the rejection as a possible mutation. A new test leaves a staging file in the created directory and checks that the rejection reports mutationMayHaveCommitted.
| return isMissingEntry(error) | ||
| ? new WorkspaceToolError('Workspace path does not exist', 'NOT_FOUND') |
There was a problem hiding this comment.
For a pathname-only workspace, a background process can replace an ancestor that was just observed as a directory with an escaping symlink before the next lstat. If the corresponding outside child is absent, that lstat returns ENOENT and this branch reports NOT_FOUND without confirming that the miss occurred inside the workspace; if the outside child exists, it instead returns INVALID_PATH, creating the outside-path existence distinction this classifier is intended to prevent. Revalidate the resolved ancestry before returning NOT_FOUND, or conservatively return INVALID_PATH when traversal cannot be descriptor-anchored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0f6d78. The classifier now records the device and inode of each real directory on the walk. After a miss, it re-runs lstat on all of them, and returns NOT_FOUND only if each is still the same non-symlink directory. Otherwise, including when an ancestor was replaced, it returns INVALID_PATH.
It also no longer follows directory symlinks at all. Any symlink in the chain, in-root or not, is INVALID_PATH, so the walk never resolves a path outside the root to decide which code to return. On held roots the lstat calls are already descriptor-anchored, with an ancestry check on each parent.
A miss is reported as NOT_FOUND only when the walk reached it through real directories that still have the same identity afterwards; any symlink in the chain stays INVALID_PATH. Cleanup syncs each removal into its parent and treats any directory it could not durably remove as a possible mutation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0f6d78e46
ℹ️ 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".
| ): Promise<WorkspaceToolError> { | ||
| const invalid = new WorkspaceToolError('Invalid workspace path', 'INVALID_PATH'); | ||
| const segments = relative(root, candidate).split(sep).filter(Boolean); | ||
| const ancestors: DirectoryIdentity[] = []; |
There was a problem hiding this comment.
For a pathname-only workspace, the root itself is never added to ancestors, so replacing the registered root with an escaping symlink before a request still leaks outside-path existence: a missing outside child reaches the first lstat catch with an empty ancestry list and returns NOT_FOUND, while an existing child is rejected as INVALID_PATH. Fresh evidence beyond the earlier ancestry thread is that the added recheck starts below root; capture and revalidate the root identity as well before returning NOT_FOUND.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4321ba9. The classifier now runs lstat on the root first and records its device and inode as the first ancestor. A root that is a symlink or not a directory is INVALID_PATH, and the root is rechecked with the other ancestors before NOT_FOUND is returned.
| } catch (cleanupError) { | ||
| if (!isMissingEntry(cleanupError)) removed = false; |
There was a problem hiding this comment.
When another process removes a newly created directory without syncing its parent, rmdir returns ENOENT here and cleanup is considered successful even though the write previously fsynced that directory's creation; a crash can therefore restore it while the rejection still claims no mutation may have committed. This catch also suppresses ENOENT thrown by syncWorkspaceDirectory after a successful rmdir. Fresh evidence beyond the earlier cleanup fix is that both cases bypass durable confirmation, so they should sync/revalidate the containing directory or mark the mutation uncertain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Obsolete as of 4321ba9. Created directories are no longer rolled back, so cleanup and its ENOENT handling are gone. They are a committed side effect, like mkdir -p. Once one exists, any later failure of the write is reported with mutationMayHaveCommitted: true and the worker quarantines. A cancellation that arrives after creation no longer interrupts the write, so the common case still finishes normally.
| if (entry == null) { | ||
| throwIfAborted(signal); | ||
| try { | ||
| await mkdir(current, 0o777); |
There was a problem hiding this comment.
If a background process renames the parent directory outside the held workspace after WorkspaceRootAccess.parent() checks its ancestry but before its mkdirat, this call creates the child through the still-open descriptor in that now-external directory. The later pathname-based realpath only detects the problem after the mutation, and cleanup cannot reach the child through the old pathname. Fresh evidence beyond the earlier parent-creation thread is that the held descriptor itself can be moved after its one-time ancestry check; creation must remain atomically rooted or retain enough descriptor state to remove and quarantine any externally installed child.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly addressed in 4321ba9; the remaining part is a property of the shared held-root primitive, not something this PR adds.
The window you describe applies to every descriptor-anchored mutation in WorkspaceRootAccess: parent() checks ancestry once and then acts through the held fd. The existing write path already relies on it. It stages .librechat-code-*.tmp with openat through the same held parent and installs it with renameat, so a parent moved outside the root between the ancestry check and the call places a file outside today. mkdirat here has the same exposure, and only for an empty directory. Closing the window generally would mean re-proving ancestry after each *at call inside root-access.ts, which belongs in its own change covering every caller.
What this PR does change is what happens when the escape is detected. The realpath check after mkdir throws INVALID_PATH, and because created directories are now a committed side effect, that failure is reported with mutationMayHaveCommitted: true. The worker therefore quarantines instead of reporting an atomic rejection. That is the "quarantine any externally installed child" outcome, without trying to remove anything through a path that no longer reaches it.
| let removed = true; | ||
| for (let index = created.length - 1; index >= 0; index -= 1) { | ||
| try { | ||
| await rmdir(created[index]); |
There was a problem hiding this comment.
Cleanup records only path strings, so if another process removes a directory created by this write and recreates an empty directory at the same path, this rmdir deletes the other process's replacement and then reports the rejected write as atomic. Fresh evidence beyond the earlier nonempty-cleanup discussion is that rmdir protects concurrent content but not an empty replacement inode; retain the created directory identity and avoid removing a path that no longer names the write-owned directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Obsolete as of 4321ba9. Cleanup was removed: created directories are never deleted, so a same-path empty replacement created by another process is never touched. A failure after creation is reported as a possible mutation instead.
| if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; | ||
| } | ||
| entry = await lstat(current); | ||
| if (createdHere) await syncWorkspaceDirectory(dirname(current)); |
There was a problem hiding this comment.
When lstat observes a missing ancestor but mkdir then returns EEXIST because another process just created it, createdHere remains false and this conditional skips syncing the new entry into its parent. The write can subsequently sync descendants and the final file yet still lose the entire raced ancestor after an acknowledged write if the machine crashes. Fresh evidence beyond the earlier parent-sync thread is that its fix covers only this process's successful mkdir; sync the containing directory before relying on the directory accepted after EEXIST.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4321ba9. Whenever lstat found the ancestor missing, its parent is now synced after the directory is confirmed, whether this write's mkdir created it or a racing mkdir did (EEXIST). So the write never acknowledges a file under an entry that was not synced.
Directories a write creates are no longer rolled back. Once one exists the write finishes despite cancellation, and any later failure is reported as a possible mutation. A raced EEXIST parent is synced too, and the NOT_FOUND ancestry recheck now includes the workspace root.
|
Codex Review: Didn't find any major issues. Hooray! 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". |
1 participant
Summary
Agents often get
INVALID_PATH("Invalid workspace path") for a well-formed path, and conclude the path format is wrong. Over 7 days on demo.librechat.ai, 57 workspace file-tool calls failed with 422INVALID_PATH. Only one of them involved a real path-safety problem; the rest were well-formed paths to files that did not exist:read_filecalls on log files that a background command had not written yet (.checks/tests.log,.verification/static.log). Several succeeded when the agent read the same path again later.packages/client/src/components/Spinner.tsxwhen the file lives undersvgs/.create_filecalls into a directory that did not exist yet:packages/dev-tools/package.jsonin a new package,__fixtures__/edit-worker.cjs,Handoffs/Notice.tsx.The cause is in the worker.
readConfinedFileBuffer,editWorkspaceFile,searchWorkspaceandlistWorkspaceFilesfold every filesystem error, includingENOENT, intoINVALID_PATH.atomicWriteConfinedFiletreats a missing parent directory the same way. So "this file is not there yet" and "this path escapes the workspace" produce the same error.This PR makes two changes.
NOT_FOUNDfor absent paths. When a read, edit, preview, search scope or listing scope does not exist, the worker walks the path from the root withlstat. It reportsNOT_FOUND("Workspace path does not exist") only when the walk reaches the missing entry through real directories, root included, and those directories are unchanged (same device and inode) when rechecked after the miss. Everything else keepsINVALID_PATH: any symlink in the chain, a file used as a directory, an ancestor replaced mid-walk, traversal and absolute paths. The split therefore reveals nothing about paths outside the root. A search that skips a file which vanished mid-scan treatsNOT_FOUNDlikeINVALID_PATH.write_filecreates missing parent directories. Before installing the file, the worker creates each missing ancestor beneath the workspace or lane root, one level at a time:mkdirandlstatgo throughmkdirat/openatfrom the held root fd, and each parent descriptor is checked for root ancestry, so a swapped symlink cannot redirect creation outside the root. A pathname-only root (legacy--directorywithout identity) creates nothing and reports the missing parent asNOT_FOUND.INVALID_PATH, as before. A write still never goes through a symlinked directory, even one inside the root.lstatand themkdir, is synced into its parent before the write is acknowledged.confirmInstalledMutationstill syncs the final parent.0o777minus the process umask, so they get the same permissions asmkdir -p. The existing 0o700 default stays for every other caller of themkdiradapter.mkdir -p, and are never rolled back. A cancellation that arrives after they exist does not stop the write. Any later failure of the write (a storage error, a raced conflict, or a created directory resolving outside the root) is reported as a possible mutation, so the worker quarantines instead of claiming an atomic rejection. A write cancelled or rejected before creating anything stays atomic.Compatibility
NOT_FOUNDis a new settlement error code, and an older Code API rejects any settlement whose code it does not know (isSettlementreturns 400 "Invalid bridge settlement"). Three things keep mixed versions working:supportedWorkspaceToolErrorCodes: ['NOT_FOUND']in its registration response./workspace-tools/executereturnsNOT_FOUNDas HTTP 422, the statusINVALID_PATHalready uses, so existing LibreChat clients take the same path-error branch.NOT_FOUNDonly after the current registration advertised it. Otherwise it settles withINVALID_PATHand keeps the new message ("Workspace path does not exist"). An older Code API can still validate the settlement, and the agent still sees the clearer text.NOT_FOUNDgets a 400 anyway, for example from an older replica during a rolling deploy, the worker resends it once withINVALID_PATH.WORKSPACE_TOOL_ERROR_CODE_FALLBACKSinprotocol.tsis the single source of that mapping.New Code API with an older worker: nothing changes. Older workers never send
NOT_FOUND, and they ignore the extra registration field.LibreChat will read
NOT_FOUND, and the legacyINVALID_PATHfrom older workers, to give agents a specific message in a separate PR.Testing
packages/codenpm teston Node 24.16.0 in WSL: 749 tests. The only failures are the same 9 PTC-watchdog and credential/identity-storage tests that fail identically on unmodifiedmainin this environment (they assert POSIX permission behavior that/mnt/cand this kernel do not provide).workspace.test.ts:NOT_FOUNDfor a missing file and a missing nested directory, across read, edit, preview, search and list.INVALID_PATHfor the same shapes through an in-root or escaping symlink, a dangling symlink, and a file used as a directory.NOT_FOUNDand creates nothing.linked-worktrees.test.ts: in a descriptor-bound lane, a write createspackages/new-package/inside the lane only, a missing log readsNOT_FOUND, and a symlink from the lane to the checkout is refused for both writes and reads.workspace-worker.test.ts:NOT_FOUNDis sent only when registration advertised it and falls back toINVALID_PATHotherwise; a refusedNOT_FOUNDsettlement is resent withINVALID_PATH; a refused legacy settlement is not retried.protocol.test.ts: every added code is valid and maps to a legacy code.servicebun test src/workspace-tools/router.test.ts src/bridge/router.test.ts: 73 pass, coveringNOT_FOUNDandINVALID_PATHmapped to 422 and the registration advertisement.servicefullbun run test: the 6 failures are timing-sensitive Redis/backend tests that pass when rerun alone and fail intermittently onmaintoo.bun run buildsucceeds.tsc --noEmitshows only errors that already exist onmain.