🏓 feat: Dispatch Worker-Local Workspace Tools - #89
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. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danny-avila
changed the title
0af9c60 to
1c433da
Compare
1c433da to
c3a53a3
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: c3a53a3c39
ℹ️ 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".
| (options.workspaceTools != null && | ||
| JSON.stringify(options.workspaceTools.capabilities) !== | ||
| JSON.stringify(options.capabilities.workspaceTools)) |
There was a problem hiding this comment.
When a library consumer supplies a custom WorkspaceToolExecutor, two valid and semantically identical capability objects can have different property insertion orders, causing these JSON.stringify results to differ and the constructor to reject the worker before registration. For example, an executor can construct { operations, workspaces, protocolVersion } while the advertised object uses { protocolVersion, operations, workspaces }; compare the fields structurally or derive the advertised capabilities directly from the executor instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c9c5b4. Worker construction now compares the protocol version and ordered operation/workspace fields structurally, so object property insertion order no longer affects valid capability matching. Added a separate-object-order regression.
| payload = await this.options.workspaceTools.execute( | ||
| workspaceRequest, | ||
| executionController.signal, | ||
| ); |
There was a problem hiding this comment.
When a custom workspace executor returns normally after the assignment deadline or after remote cancellation while ignoring or belatedly observing the supplied signal, this branch proceeds to create a fulfilled settlement without checking executionController.signal.aborted or the deadline again. An already-expired settlement is still sent because settleWithRetry begins its first request before its zero-delay deadline timer fires, so deadline and cancellation fencing can be bypassed; check the execution signal and deadline immediately after execute resolves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3c9c5b4. The worker now rechecks both the execution abort signal and absolute local deadline immediately after a workspace executor resolves, before constructing a fulfilled settlement. Added a custom executor regression that returns after deadline and verifies only a rejected settlement is sent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c9c5b4a6a
ℹ️ 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 (executionController.signal.aborted) { | ||
| throw ( | ||
| executionController.signal.reason ?? | ||
| new DOMException('aborted', 'AbortError') | ||
| ); | ||
| } |
There was a problem hiding this comment.
When a cancellation poll has received its response but its promise continuation is queued as the workspace executor resolves, this check still sees an unaborted signal and constructs a fulfilled settlement. The subsequent cancellationController.abort() cannot undo an already-fulfilled poll, and await cancellationWatcher can then abort executionController without the settlement being reconsidered. Fresh evidence in this head is that the watcher is drained only after the fulfilled settlement is constructed, with no second signal check, so this race can still report a cancelled workspace operation as fulfilled; recheck the execution signal after draining the watcher before settlement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8cdae. Successful execution now stops and awaits the cancellation watcher before choosing a fulfilled settlement, then rechecks the execution abort signal. Added a deterministic race regression where executor and completed cancellation poll continuations are queued together; only a rejected settlement is emitted.
| name: | ||
| option(args, '--workspace-name') ?? | ||
| process.env.LIBRECHAT_CODE_WORKSPACE_NAME?.trim() ?? | ||
| basename(resolve(workerDirectory)), |
There was a problem hiding this comment.
When --worker-dir is a filesystem root such as / (or the equivalent platform root), basename(resolve(workerDirectory)) returns an empty string. That empty default name fails isValidBridgeWorkspaceToolCapabilities, so an otherwise valid directory cannot be registered unless the operator supplies --workspace-name; omit the optional name or fall back to the workspace ID when the basename is empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8cdae. The CLI now derives the workspace ID first and uses it when an existing directory has no basename. Added a live registration test using filesystem root as --worker-dir and verified the advertised workspace name falls back to the ID.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8cdae90d
ℹ️ 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".
| cancellationController.abort(); | ||
| await cancellationWatcher; |
There was a problem hiding this comment.
When the cancellation response headers have arrived but its JSON body is still being read as workspace execution completes, aborting cancellationController also aborts the poll's response.json(). watchCancellation then catches the error and returns because its signal is aborted, so executionController remains unaborted and the worker sends a fulfilled result despite the server having returned cancelled: true. Fresh evidence in this head is that the new drain sequence aborts the in-flight request before awaiting it; stop future polls without aborting the current response, or await that response before deciding the settlement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6811caf. The stop-watcher signal now interrupts delays and prevents future polls without aborting an active response body; the execution/deadline signal still aborts the active poll. Added a regression with received headers and a delayed, abort-sensitive cancellation JSON body; the worker drains it and emits only a rejected settlement.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6811caf7fe
ℹ️ 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".
| cancellationController.abort(); | ||
| await cancellationWatcher; |
There was a problem hiding this comment.
When an active cancellation response finishes only after the local deadline—for example, because its body parsing blocks the event loop past that deadline—this await can resume before the deadline timer callback runs. The last absolute Date.now() check occurs before the drain, so the code can construct a fulfilled settlement and then clear the overdue timer; settleWithRetry can likewise begin its request before its zero-delay timer fires. Fresh evidence in the current head is that the active response is now intentionally drained after the last absolute deadline check, so recheck localDeadlineAtMs after this await.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1381217. The worker now rechecks the absolute local deadline immediately after the cancellation watcher drains and before it can choose a fulfilled settlement. Added a regression whose body parser blocks across the deadline before the timer callback can run.
| executionController.signal.addEventListener('abort', abortPoll, { | ||
| once: true, | ||
| }); |
There was a problem hiding this comment.
When workspace execution finishes after cancellation-response headers arrive and that response is a 404, detaching the stop signal here lets the response finish, but watchCancellation then sees that its stop signal is aborted and returns before its existing 404 branch can abort execution. The worker consequently sends a fulfilled result even though this endpoint explicitly treats 404 as cancellation; handle the 404 error before the stop-signal early return.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1381217. watchCancellation now handles a 404 by aborting execution before applying the stop-future-polls early return. Added a delayed 404-body regression proving the drained response produces only a rejected settlement.
|
Codex Review: Didn't find any major issues. 🚀 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 138121754e
ℹ️ 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".
| (basename(resolve(workerDirectory)) || workspaceId), | ||
| root: workerDirectory, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
When --worker-dir points to a valid directory whose basename is whitespace-only or longer than BRIDGE_WORKSPACE_NAME_MAX_LENGTH (for example, a 129-character Linux directory name), this default is passed as the workspace name and LocalWorkspaceTools.create rejects the registration. In this head the fallback only handles an empty basename, so these directories still require an otherwise unnecessary explicit --workspace-name; validate the derived basename and fall back to the workspace ID when it cannot be advertised.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4ccea0df. Automatically derived workspace names are now accepted only when non-whitespace and within the protocol length limit; otherwise the CLI advertises the configured workspace ID. Explicit invalid names still fail validation. The package suite passes all 142 tests, including a real whitespace-named directory registration regression.
4ccea0d to
9a30d8f
Compare
2 participants
Summary
I connected the local workspace executor to the bridge worker and CLI so a configured BYOM worker can advertise and settle fenced workspace-tool assignments without sending its repository through the sandbox API.
workspace_toolassignment kind while preserving the existing sandbox assignment shape.librechat-code run --worker-dir <path>with configurable public workspace ID and display name.Depends on #88.
Change Type
Testing
npm testinpackages/code: 122 tests passed.npm run buildinservice: build completed with existing Rollup/TypeScript warnings.Test Configuration:
Checklist