🎖️ feat: Expose Authenticated Workspace Tool API - #90
Conversation
|
@codex review |
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: 8e0d9b8986
ℹ️ 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".
| import { createWorkspaceToolsRouter } from './router'; | ||
|
|
||
| const router = Router(); | ||
| router.use(executionLimiter); |
There was a problem hiding this comment.
Because this router is mounted with v1.use(workspaceToolsRouter) before the existing service routers, this pathless middleware runs for every authenticated /v1 request, including uploads, downloads, and unknown routes. Existing /exec and /exec/programmatic requests then encounter executionLimiter again in their own routers, consuming two quota units per request and effectively halving the configured execution limit; unrelated file operations can also exhaust that quota. Attach the limiter specifically to the workspace execution route instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9df8ece. The execution limiter is now mounted only at /workspace-tools/execute, so unrelated /v1 and existing execution routes do not consume an extra unit.
| result.startLine === startLine && | ||
| Number.isSafeInteger(result.endLine) && | ||
| Number(result.endLine) >= startLine - 1 && | ||
| Number(result.endLine) < startLine + maxLines && |
There was a problem hiding this comment.
When a worker returns a malformed read_file settlement, this only bounds the worker-reported endLine; it never checks that content contains the corresponding number of lines. For example, a request with maxLines: 1 can return thousands of newline-delimited lines while claiming endLine: startLine, and the public endpoint will accept and forward the result up to the 1 MiB byte limit, bypassing both the caller's requested bound and the documented 500-line ceiling. Derive the line count from content and require it to agree with the reported range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9df8ece. Fulfilled read results now derive the content line count and require it to agree with the reported range and requested maximum; a multi-line result claiming one line is covered by the package test.
|
|
||
| function bridgeStoreStatus(error: BridgeStoreError): number { | ||
| if (error.code === 'WORKER_UNAUTHORIZED') return 403; | ||
| if (error.code === 'ASSIGNMENT_INVALID') return 400; |
There was a problem hiding this comment.
For a syntactically valid client request, dispatchWorkspaceTool raises ASSIGNMENT_INVALID when the remote worker returns a result that fails validation, but this mapping reports that condition as HTTP 400. The request body has already passed isWorkspaceToolRequest, so clients are incorrectly told to fix their request and may suppress retries or fallback when the actual fault is the selected worker or protocol compatibility. Use a distinct result-validation error or map this case to an upstream failure such as 502.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9df8ece. Invalid fulfilled worker results now use a distinct RESULT_INVALID store error mapped to HTTP 502, while invalid client assignments remain HTTP 400. Focused store/router tests pass.
danny-avila
changed the title
c06021c to
9df8ece
Compare
|
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". |
9df8ece to
c3a53a3
Compare
6ab900b to
220a3cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 220a3cde0e
ℹ️ 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".
| new TextEncoder().encode(content).byteLength <= | ||
| BRIDGE_WORKSPACE_READ_MAX_BYTES && |
There was a problem hiding this comment.
When reading a supported UTF-16 file containing multibyte characters, the worker caps the raw file at 1 MiB before decodeWorkspaceText, but this check measures the decoded string as UTF-8. For example, an approximately 800 KB UTF-16 CJK file produces roughly 1.2 MB of UTF-8 text, so the bundled worker successfully executes the read and then Code API rejects its valid settlement as RESULT_INVALID with HTTP 502. Either bound the worker's decoded output using the same encoding or allow the maximum expansion here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aaf5b8b. The worker now applies the 1 MiB boundary again after decoding by measuring the UTF-8 result, matching Code API’s settlement validator. The package suite includes a UTF-16 CJK expansion regression test.
| hasOnlyKeys(candidate, WORKSPACE_SEARCH_MATCH_KEYS) && | ||
| isSafePortableRelativePath(candidate.path) && |
There was a problem hiding this comment.
When a search_text request limits the search to a file or subtree via request.path, a malformed or incompatible worker can return matches from anywhere else in the workspace because this only checks that each match path is a portable relative path. The public endpoint then accepts results that violate the originating request's scope—for example, a search under src may report secrets.env. Require match paths to equal a requested file or remain within the requested directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aaf5b8b. Search result validation now normalizes portable relative paths and requires every match to equal or remain beneath the request path. Tests reject sibling-prefix and out-of-scope paths.
| v1.use('/bridge', bridgeRouter); | ||
| v1.use(isLocalMode ? localAuth : apiKeyAuth); | ||
|
|
||
| v1.use(workspaceToolsRouter); |
There was a problem hiding this comment.
When Code API is run through service/Dockerfile.local, that image builds src/local-api.ts, which still mounts only the bridge, authentication, service, and programmatic routers. Consequently a local deployment configured with the supported remote-bridge backend returns 404 for /v1/workspace-tools/execute, even though the new router is mounted in the other two server entrypoints. Mount workspaceToolsRouter in local-api.ts as well so this feature is available in the repository's local deployment and development server.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aaf5b8b. The local API entrypoint now mounts the workspace tools router after local authentication. Focused service tests and the production bundle pass, and a live local API plus Redis plus outbound worker smoke exercised both search and read through the workspace-tools endpoint.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaf5b8b47a
ℹ️ 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 ( | ||
| hasOnlyKeys(candidate, WORKSPACE_SEARCH_MATCH_KEYS) && | ||
| isSafePortableRelativePath(candidate.path) && | ||
| isWithinRequestedPath(candidate.path, request.path) && |
There was a problem hiding this comment.
When request.path names an in-workspace directory symlink (for example, alias -> src), the bundled searchWorkspace resolves it with realpath and returns a match such as src/a.txt; this lexical check instead requires the result to remain beneath alias, so dispatchWorkspaceTool rejects the worker's own valid settlement as RESULT_INVALID and the endpoint returns HTTP 502. Preserve the requested prefix in worker results or make validation accommodate canonical in-root aliases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53d5b60. The worker now maps canonical search candidates back into the caller’s normalized in-workspace path namespace, so a search under alias returns alias/app.ts while confinement still reads the canonical in-root file. A real symlink-scoped search regression covers this.
| const candidate = match as Record<string, unknown>; | ||
| return ( | ||
| hasOnlyKeys(candidate, WORKSPACE_SEARCH_MATCH_KEYS) && | ||
| isSafePortableRelativePath(candidate.path) && |
There was a problem hiding this comment.
When the bundled worker runs on Windows, this rejects nested matches because isSafePortableRelativePath forbids backslashes while listSearchCandidates invokes rg without --path-separator and returns its paths unchanged. The installed rg --help documents, “This defaults to your platform's path separator, which is / on Unix and \ on Windows.” Consequently a normal result such as src\index.ts becomes RESULT_INVALID and HTTP 502; pass --path-separator / or normalize returned candidate paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53d5b60. Candidate enumeration now invokes ripgrep with --path-separator /, keeping advertised match paths portable and compatible with the protocol validator on Windows.
| typeof candidate.text === 'string' && | ||
| candidate.text.length <= BRIDGE_WORKSPACE_SEARCH_TEXT_MAX_LENGTH |
There was a problem hiding this comment.
When a malformed or incompatible worker returns a stale fulfilled search result for the same workspace, this validates only the preview's type and length, so a match with text: "completely unrelated" is accepted for a request whose query is needle and then returned as a successful search result. Require every preview to contain request.query before accepting the settlement; the bundled worker already centers each literal match in its preview.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 53d5b60. Search result validation now requires every bounded preview to contain the originating literal query. The validation regression rejects an unrelated stale preview.
aaf5b8b to
2720e4c
Compare
53d5b60 to
fd91255
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd91255f23
ℹ️ 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".
| signal: controller.signal, | ||
| }); | ||
| if (settlement.status === 'rejected') { | ||
| res.status(422).json({ |
There was a problem hiding this comment.
When a valid search reaches a worker but ripgrep is unavailable or the search exceeds its internal deadline, LocalWorkspaceTools raises SEARCH_UNAVAILABLE or SEARCH_TIMEOUT, but the worker serializes only the message in a rejected settlement and this unconditional branch reports it as 422. Clients are therefore told the request is unprocessable and may suppress retries for an upstream availability or timeout failure; preserve a bounded worker error code and map these cases to an appropriate 5xx response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1b499b. WorkspaceToolError codes now cross the bridge as a bounded protocol enum; SEARCH_TIMEOUT maps to HTTP 504 and SEARCH_UNAVAILABLE to 503, while older workers without a code remain compatible. Package, focused service, production build, and live outbound-worker verification pass.
| }; | ||
| res.once('close', abortClosedResponse); | ||
| try { | ||
| const settlement = await options.store.dispatchWorkspaceTool({ |
There was a problem hiding this comment.
When gracefulShutdown has set the service shutdown flag but the HTTP process remains reachable while workers and queue connections are being drained, this handler can still create and lease a new workspace assignment. Unlike /exec and /exec/programmatic, it never checks checkServiceShutDown() before dispatch, so the connection can be closed or the process can exit before this newly accepted request settles; add the same 503 lifecycle gate before calling the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1b499b. The workspace endpoint now checks the service shutdown gate before validation or dispatch; a regression test proves the store is never called and the endpoint returns 503.
| (result.truncated === true | ||
| ? Number.isSafeInteger(result.nextStartLine) && | ||
| Number(result.nextStartLine) === Number(result.endLine) + 1 |
There was a problem hiding this comment.
When a worker returns an empty page with endLine equal to startLine - 1, this branch accepts truncated: true with nextStartLine equal to the original startLine. For example, startLine: 1, empty content, endLine: 0, and nextStartLine: 1 passes every validation check, so a client following the advertised cursor can repeat the same request indefinitely. Require a truncated result to contain at least one reported line or otherwise ensure nextStartLine is strictly greater than the requested start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1b499b. Truncated read settlements must now advance nextStartLine beyond the requested start. The empty page/endLine 0/nextStartLine 1 case is explicitly rejected by the package regression suite.
2 participants
Summary
I added an authenticated Code API data plane for worker-local workspace reads and searches, building on #89.
POST /v1/workspace-tools/executethrough the authenticated principal and existing remote-bridge worker selection.Depends on #89.
Change Type
Testing
npm testinpackages/code: 123 passed.service: 67 passed.npm run buildinservice; the build completed with the existing repository warnings.@librechat/codeworker using this checkout as workspaceprimary.read_fileandsearch_textresults across the bridge.../secret.txtis rejected with HTTP 400.Test Configuration:
CODEAPI_SANDBOX_BACKEND=remote-bridgeCODEAPI_BRIDGE_AUTH_MODE=staticfor isolated local verificationChecklist