🗺️ feat: List Attached Workspace Files - #92
Conversation
98218dc to
25e4920
Compare
25e4920 to
a2a25e1
Compare
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. |
a2a25e1 to
190bb9b
Compare
![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: 190bb9b943
ℹ️ 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".
| 'path', | ||
| '--null', | ||
| '--', | ||
| canonicalListPath, |
There was a problem hiding this comment.
If workspace contents can change concurrently, the path checked by realpath() can be replaced with an escaping symlink before this rg process opens canonicalListPath, causing filenames outside the registered root to be returned. The rg --help description limits --follow/--no-follow to links encountered “while traversing directories”; an explicit command-line directory that is a symlink is still followed (for example, rg --files --no-follow -- link lists files below link). This violates the documented symlink-escape boundary, so traversal must remain tied to the verified directory inode or each emitted path must be confined before it is returned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d524fb3. Every representable candidate is now resolved and checked against the canonical workspace root after enumeration and before it is returned, so a replaced explicit directory cannot expose an escaping result.
| const portablePath = sep === '\\' ? path.split(sep).join('/') : path; | ||
| const normalizedPath = portablePath.startsWith('./') | ||
| ? portablePath.slice(2) | ||
| : portablePath; | ||
| paths.push( |
There was a problem hiding this comment.
On POSIX workspaces, a valid filename containing a backslash (for example, foo\bar) is emitted unchanged here because sep is /. The resulting list then fails isWorkspaceToolResult(), whose portable-path check rejects every backslash, so dispatchWorkspaceTool() turns the otherwise fulfilled settlement into RESULT_INVALID and the entire listing request fails. Skip or otherwise encode filesystem paths that cannot satisfy the result contract before adding them to paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d524fb3. Unrepresentable filesystem names are filtered before result construction, with a POSIX backslash-filename regression test.
| this.capabilities = { | ||
| protocolVersion: BRIDGE_PROTOCOL_VERSION, | ||
| operations: ['read_file', 'search_text'], | ||
| operations: ['read_file', 'search_text', 'list_files'], | ||
| workspaces, |
There was a problem hiding this comment.
When this independently installed worker CLI is upgraded before its Code API deployment, it still identifies the capability payload as protocol version 1 but now always advertises three operations. The preceding v1 server validates workspace capabilities with operations.length <= 2 and accepts only read_file or search_text, so it returns HTTP 400 for every registration; the worker then remains in its reconnect loop and cannot lease even ordinary sandbox work. Preserve a registration-compatible capability set until server support is known, or introduce explicit protocol negotiation for this operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d524fb3 with explicit registration negotiation. A new worker first advertises the v1-compatible read/search subset; a supporting Code API returns its workspace operation set and the worker then re-registers with list_files. Older servers keep ordinary sandbox and existing workspace operations available. Both paths have regression tests.
190bb9b to
d524fb3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d524fb3909
ℹ️ 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".
| this.registrationCapabilities = this.options.capabilities; | ||
| try { | ||
| registration = await register(this.registrationCapabilities); |
There was a problem hiding this comment.
When the server supports list_files and the compatible registration consumes a substantial part of registrationTransportTimeoutMs, this second request shares the AbortController and timer started before the first request. For example, with the default 10-second timeout, two otherwise acceptable 6-second responses always abort the second request after about 4 seconds; the catch resets to compatible capabilities, and the worker repeats the same failing sequence without ever leasing work. Give each registration request its own transport timeout or handle exhaustion by retaining the successful compatible registration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1dce623. If optional list_files promotion times out or otherwise fails, the worker now retains the already-successful compatible registration and proceeds with the baseline capability set; caller cancellation still aborts registration. Added a stalled-promotion regression test.
| for (const candidate of candidates) { | ||
| let canonicalPath: string; | ||
| try { | ||
| canonicalPath = await realpath( | ||
| resolveWorkspacePath(root, candidate.filesystemPath), |
There was a problem hiding this comment.
After rg exits, its 10-second timer is cleared, but this loop can perform up to 1,000 serial realpath calls without consulting the abort signal or any deadline. On a slow or network-mounted workspace, or when the caller aborts during verification, list_files can therefore continue well past its timeout or assignment deadline and keep the worker occupied; direct LocalWorkspaceTools callers may even receive a normal result after aborting. Carry the overall listing deadline into this loop and check cancellation and timeout between candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1dce623. The listing deadline now begins before initial path canonicalization and is enforced, together with cancellation, around every candidate realpath/stat verification rather than ending when ripgrep exits.
| canonicalPath = await realpath( | ||
| resolveWorkspacePath(root, candidate.filesystemPath), | ||
| ); |
There was a problem hiding this comment.
When request.path directly names an in-workspace FIFO, socket, or device, ripgrep emits that positional target even though normal directory traversal skips it; this was confirmed against rg --help (--files “Print[s] each file that would be searched”) and by running the exact flags against a FIFO. Because this validation only resolves and confines the candidate without checking its type, list_files reports a path that read_file and search_text reject as non-regular. Stat each resolved candidate and retain only regular files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1dce623. Each confined candidate is statted within the same deadline and only regular files are returned. Added an explicit FIFO regression test.
d524fb3 to
1dce623
Compare
|
@codex review Please review the current PR head 1dce623. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. The second-round findings are addressed, and I completed the cross-seam review of registration fallback, mixed-version rollout, listing confinement, deadline/cancellation propagation, result bounds, and file-type consistency. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dce623726
ℹ️ 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".
| candidates.push({ filesystemPath: normalizedPath, resultPath }); | ||
| }; | ||
|
|
||
| child.stdout.setEncoding('utf8'); |
There was a problem hiding this comment.
On POSIX, setEncoding('utf8') replaces invalid filename bytes with U+FFFD before isSafePortableRelativePath() checks representability. If a workspace contains both an unrepresentable name such as raw byte 0xff and a valid file named �, an rg result for the former is converted into the latter; verification then stats the valid file and may return it even when that file was excluded by ignore rules. Fresh evidence in this head is that the raw rg --null stream is decoded at this line before validation; parse it as buffers and reject byte sequences that do not round-trip as UTF-8.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0383e8069a7b9fbf880c78ae1bc02b179afebf4. Listing output now remains byte-oriented through NUL framing and uses fatal UTF-8 decoding per filename, so invalid byte sequences are rejected before any filesystem verification can alias them to U+FFFD. Added a Linux-capable regression covering the invalid-byte/valid-U+FFFD collision.
There was a problem hiding this comment.
Commit reference correction: the published fix is d0383e8.
| hasOnlyKeys(result, WORKSPACE_LIST_RESULT_KEYS) && | ||
| Array.isArray(result.paths) && | ||
| result.paths.length <= maxResults && | ||
| new Set(result.paths).size === result.paths.length && |
There was a problem hiding this comment.
When a custom or buggy worker returns aliases such as src/app.ts and src/./app.ts, both entries pass the portable-path and requested-subtree checks, while this raw-string Set treats them as distinct. The Code API therefore accepts duplicate references to the same logical file, allowing duplicates to consume the result bound and reach callers despite the result contract's duplicate validation; either require canonical result paths or deduplicate their normalized forms.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d0383e8069a7b9fbf880c78ae1bc02b179afebf4. Result validation now deduplicates normalized portable paths, so aliases such as src/app.ts and src/./app.ts cannot represent the same logical file twice. Added the reported pair as a protocol regression test.
There was a problem hiding this comment.
Commit reference correction: the published fix is d0383e8.
|
@codex review Please review the current PR head d0383e8069a7b9fbf880c78ae1bc02b179afebf4. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. The third-round findings are addressed by enforcing one canonical portability invariant at both seams: byte-exact fatal UTF-8 decoding at enumeration and normalized duplicate rejection at protocol validation. |
|
@codex review Correction: please review the current PR head d0383e8. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. The third-round findings are addressed by enforcing one canonical portability invariant at both seams: byte-exact fatal UTF-8 decoding at enumeration and normalized duplicate rejection at protocol validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0383e8ce0
ℹ️ 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 (rawPath.length === 0 || stoppedForLimit) return; | ||
| let path: string; | ||
| try { | ||
| path = new TextDecoder('utf-8', { fatal: true }).decode(rawPath); |
There was a problem hiding this comment.
At the requested head d0383e8ce0165368eed0259866406f4d6bf05269, an explicitly listed top-level path beginning with U+FEFF is emitted by rg with EF BB BF at byte zero, but TextDecoder strips that sequence by default. The new decoder therefore changes foo into foo, causing verification to resolve the wrong sibling (potentially returning an ignored file to direct callers) or causing server-side subtree validation to reject the result. Set ignoreBOM: true or enforce an exact decode/re-encode round trip so valid UTF-8 filenames remain byte-exact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5c716c773317c8cd9a4f0a9138e3c9562fb2f199. Filename decoding now preserves a leading BOM and requires the decoded string to re-encode to the exact original bytes, while still rejecting malformed UTF-8. Added the reported BOM/ignored-sibling regression.
There was a problem hiding this comment.
Commit reference correction: the published fix is 5c716c7.
|
@codex review Please review the current PR head 5c716c7. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. Filename enumeration now enforces byte-exact valid UTF-8, including preservation of leading BOM code points; normalized result aliases are rejected at the protocol boundary. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c716c714e
ℹ️ 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".
| regularFile = ( | ||
| await withinListDeadline(stat(canonicalPath), signal, deadline) | ||
| ).isFile(); |
There was a problem hiding this comment.
When request.path is a symlink to a regular file inside the workspace, the initial realpath() causes ripgrep to enumerate the target, but the result is remapped back to the symlink alias. Because this check stats only the canonical target, list_files returns that alias even though read_file opens the reported path with O_NOFOLLOW and rejects it as INVALID_PATH. Check the final component in the reported namespace or omit such aliases so listings contain usable file paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b602bb2. Listing now validates the final component in the reported namespace with lstat and requires that its canonical target exactly matches the enumerated file, so an explicit file symlink alias is omitted while safe directory-alias descendants remain usable.
| 'path', | ||
| '--null', | ||
| '--', | ||
| canonicalListPath, |
There was a problem hiding this comment.
When request.path names a directory or file excluded by .ignore or .gitignore, passing canonicalListPath as ripgrep's positional path makes the exclusion ineffective. I checked ripgrep 15.1.0's rg --help, whose <PATH> documentation states: “File paths specified on the command line override glob and ignore rules.” Consequently, a root listing can hide vendor/, while listing with path: "vendor" reveals the entire ignored subtree and may traverse a very large dependency tree. Enumerate from an unignored ancestor and filter to the requested subtree, or independently enforce ignore matching for the explicit target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b602bb2. Ripgrep now always enumerates from the workspace root and uses a glob to narrow to the canonical requested subtree, so positional paths no longer override .ignore/.gitignore policy. Added an explicitly ignored subtree regression test.
2 participants
Summary
I added bounded, deterministic file discovery for worker-local attached workspaces so agents can inspect an existing project, Git repository, or empty directory without knowing filenames in advance. This PR is stacked on #90.
list_filesrequest and result contracts to@librechat/code.rg --fileswithout a shell, preserve normal ignore behavior, avoid symlink following, enforce a ten-second deadline, and support cancellation.Change Type
Testing
npm testinpackages/code: 126 tests passed.src/app.tsandsrc/worker.ts; an empty directory returned an empty bounded result.Test Configuration:
127.0.0.1:23117127.0.0.1:26385@librechat/codeoutbound worker with--worker-dirpointing to a non-Git temporary directoryChecklist