🌱 feat: Create an Explicit Default Workspace - #93
Conversation
e6252a3 to
4f1886d
Compare
4f1886d to
4971e70
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. |
4971e70 to
f2c1447
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: f2c1447a83
ℹ️ 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 readableName === value | ||
| ? readableName | ||
| : `${readableName}-${createHash('sha256') |
There was a problem hiding this comment.
When a valid ID contains :, the encoded result can itself be another valid ID that takes the unhashed branch. For example, vm:a and vm_a-2d4fcea9e21e004d both pass the bridge ID pattern and both map to vm_a-2d4fcea9e21e004d, so workers under the same home—or workspace IDs under one worker—can unexpectedly share the same persistent default workspace. Use disjoint encodings, such as hashing every value or prefixing encoded names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fbcba99. Default workspace components now use disjoint raw- and exact base64url encoded- namespaces, while the pre-existing bridge identity filename behavior remains unchanged. Added the reported collision pair as a regression test.
| const workerDirectory = | ||
| explicitWorkerDirectory ?? | ||
| (useDefaultWorkspace | ||
| ? defaultWorkspacePath(workerId, workspaceId) |
There was a problem hiding this comment.
When LIBRECHAT_CODE_WORKER_DIR is exported as an empty or whitespace-only placeholder (or --worker-dir= is supplied), explicitWorkerDirectory becomes ''. The conflict check treats that as absent, but this nullish coalescing still selects it, so --default-workspace/LIBRECHAT_CODE_DEFAULT_WORKSPACE=true silently creates and registers no workspace. Normalize the empty value to undefined before choosing the default path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fbcba99. Worker-directory input is normalized so empty and whitespace-only CLI/environment values are absent and --default-workspace can select the application-owned path. The CLI integration test now covers a whitespace-only environment value.
f2c1447 to
fbcba99
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbcba99587
ℹ️ 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 /^[A-Za-z0-9._-]+$/.test(value) | ||
| ? `raw-${value}` | ||
| : `encoded-${Buffer.from(value, 'utf8').toString('base64url')}`; |
There was a problem hiding this comment.
On case-insensitive filesystems such as the default macOS setup supported by this package, distinct valid IDs like VM-1 and vm-1 produce raw-VM-1 and raw-vm-1, which resolve to the same directory and can share persistent workspace contents under one home. Fresh evidence in this exact head is that the replacement raw- namespace still preserves letter case, while BRIDGE_WORKER_ID_PATTERN treats these IDs as distinct; use a case-independent encoding or digest for every component.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 47d4832. Every default workspace component now uses a full lowercase SHA-256 digest, so case-insensitive filesystems cannot alias distinct case-sensitive IDs. Coverage includes the VM-1/vm-1 case-folded comparison.
| function nonEmpty(value: string | undefined): string | undefined { | ||
| return value?.trim().length ? value : undefined; | ||
| } |
There was a problem hiding this comment.
When LIBRECHAT_CODE_WORKER_DIR contains incidental leading or trailing whitespace, nonEmpty now verifies the trimmed value but returns the original string, so LocalWorkspaceTools.create calls realpath on a different path and rejects registration. The parent implementation trimmed this environment variable, so this commit regresses previously valid configurations such as LIBRECHAT_CODE_WORKER_DIR=' /srv/project '; return the normalized value for environment input while retaining the intended empty-value handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 47d4832. Environment-provided worker directories are trimmed before empty-value normalization, preserving the prior behavior; CLI-provided paths retain their literal spelling. Added a CLI regression test with leading/trailing environment whitespace.
fbcba99 to
f8f8dc3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8f8dc3e00
ℹ️ 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".
|
|
||
| The directory is created with owner-only permissions below | ||
| `~/.local/share/librechat/code/workspaces/<worker>/<workspace>`. It persists | ||
| across worker restarts so tools can create a project from scratch. The worker |
There was a problem hiding this comment.
When an operator follows this new workflow with an empty home, the registered workspace remains empty because LocalWorkspaceTools.capabilities in packages/code/src/workspace.ts only exposes read_file, search_text, and list_files; there is no operation that can create a directory or write a file. Consequently, bridge tools cannot create a project from scratch as documented here. Either add a write/create capability or state that the operator must seed the directory externally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38d0d50. The documentation now explicitly states that the current workspace surface is read-only and an empty default directory must be populated by a local process until write-capable coding tools are enabled.
| workspaceStorageName(workerId), | ||
| workspaceStorageName(workspaceId), |
There was a problem hiding this comment.
When the same host account re-pairs a worker ID for another tenant, this path remains identical because it includes only the public worker and workspace IDs. RedisBridgePairingStore.redeem creates a new stable identityId and tenant binding on every pairing, but the newly bound worker will advertise the previous identity's persisted files through read_file, search_text, and list_files; the same leak occurs when an ID is reused against another Code API deployment. Include the paired identity/deployment in the storage namespace, or explicitly clear the workspace when the security identity changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38d0d50. Default paths now include a deployment/security-identity namespace derived from the normalized Code API URL plus the paired public key (or static credential), ahead of the worker/workspace digests. Credential refresh within one pairing remains stable; re-pairing or changing deployments produces an isolated path. Tests cover both identity and deployment changes.
f8f8dc3 to
47d4832
Compare
|
@codex review Please review the current PR head 47d4832. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. Before this request I completed the cross-seam review of stable path identity, case-insensitive filesystems, input normalization, restart persistence, confinement, and both mixed-version registration directions. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
47d4832 to
f30de0f
Compare
|
Codex Review: Didn't find any major issues. Nice work! 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". |
f30de0f to
38d0d50
Compare
|
@codex review Please review the current PR head 38d0d50. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. The previous P1 is addressed by deployment/security-identity namespace isolation, and the docs now state the current read-only behavior accurately. |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
38d0d50 to
ad6130f
Compare
|
@codex review Please review the current PR head ad6130f. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. This is the same default-workspace feature and deployment/security-identity isolation fix, restacked onto #92 head d0383e8 after its canonical listing fix. |
|
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". |
a8ab8e9 to
a26f086
Compare
a26f086 to
af1b6e5
Compare
2 participants
Summary
I added an explicit application-owned workspace for BYOM workers that start without an existing repository or project directory.
--default-workspaceorLIBRECHAT_CODE_DEFAULT_WORKSPACE=trueopt-in.--worker-dir.Change Type
Testing
npm testinpackages/code(129 tests passed).Test Configuration:
Checklist