🛤️ feat: Schedule Linked Worktrees as Their Own Workspace Lanes - #270
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: b5b988a132
ℹ️ 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".
| ...(assignment.workspaceLeaseSlot === undefined | ||
| ? ['redis.call(\'SET\', KEYS[4], ARGV[1], \"PX\", ARGV[5])'] | ||
| : []), | ||
| "if #KEYS >= 10 and redis.call('EXISTS', KEYS[10]) == 1 then return -1 end", |
There was a problem hiding this comment.
When a linked-worktree lane's parent checkout is quarantined, this Lua script has already stored the assignment and pushed it onto the worker queue before returning -1. The subsequent JavaScript cleanup is not atomic with the enqueue, so an outstanding worker lease can claim and acknowledge the assignment first and begin executing a supposedly rejected operation; the caller meanwhile receives WORKSPACE_QUARANTINED, and the lane has no durable fence because the script returned before creating it. Check KEYS[10] before the assignment SET/RPUSH so quarantined-parent rejection cannot expose work to the worker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abf5349: the parent fence check now runs before the assignment is stored or queued, so a refused lane is never visible to a lease.
| linkedWorktree: { | ||
| checkoutRoot: lane.checkoutRoot, | ||
| commonGitDir: lane.commonGitDir, | ||
| readOnlyGitPaths: lane.readOnlyGitPaths, | ||
| }, |
There was a problem hiding this comment.
These options are registered on NativeProcessWorkspaceCommandSandbox, but NativeProcessWorkspaceCommandSandbox.start() explicitly reconstructs the IPC options object without linkedWorktree, so the forked NativeSrtWorkspaceCommandSandbox never receives the common Git directory or its read-only paths. Consequently lane commands use the ordinary workspace policy: mutating Git commands cannot write the linked worktree's external common Git directory, and none of the intended config, hooks, or sibling-metadata protections are installed. Include linkedWorktree in the options sent by the parent process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abf5349: linkedWorktree is now forwarded in the prepare options (covered in native-process.test.ts).
| workspaceRoot: join(source.root, LINKED_WORKTREE_DIRECTORY, worktree), | ||
| }), | ||
| workerId, | ||
| workspaceIsolationKey(selectedWorkspaceId, undefined, worktree), | ||
| incarnationId, |
There was a problem hiding this comment.
A failed or ambiguous lane execution now creates a durable guard keyed by workspaceIsolationKey(..., worktree), but the only operator recovery path, BridgeWorker.resetNativeWorkspace, accepts only a root or workspaceInstanceId and resolves only workspaceQuarantines/workspaceQuarantineResolver; the CLI likewise exposes only --reset-workspace-instance. After clearing the local file, the server-side lane fence therefore cannot be acknowledged and removed through the supported tooling, leaving that worktree name permanently unusable unless Redis is edited or the lane is renamed. Extend the reset protocol and CLI to select the linked-worktree guard and isolation key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abf5349: resetNativeWorkspace accepts a worktree, resolves the lane guard, and resets native-workspace:<lane key>. The CLI gains --reset-workspace-worktree <name>, and the README recovery steps cover lanes.
| ]; | ||
| const readOnlyGitPaths: string[] = []; | ||
| for (const candidate of candidates) { | ||
| if ((await lstat(candidate).catch(() => undefined)) != null) readOnlyGitPaths.push(candidate); |
There was a problem hiding this comment.
When a checkout starts without one of these reserved paths—for example, an operator has removed .git/hooks—the path is omitted from readOnlyGitPaths. Because the lane sandbox grants write access to the entire common Git directory, a lane command can then recreate the missing hooks, config, or info path and affect concurrently executing sibling worktrees, despite the documented read-only boundary. Include the fixed protected paths even when they do not yet exist; the later canonicalization already supports resolving missing path segments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abf5349: config, config.worktree, hooks and info are always denied. SRT blocks creation of deny paths that do not exist yet.
…icy, reset lane fences - Check the parent checkout fence before the assignment is stored or queued. - Forward the linked worktree policy to the forked native sandbox. - Protect shared Git config, hooks and info even before they exist. - Reset a lane fence with --reset-workspace-worktree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf5349a23
ℹ️ 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".
| const parent = workspaceIsolationParent(workspaceKey); | ||
| if (parent == null) return; |
There was a problem hiding this comment.
When a lane mutation has an ambiguous outcome, only that lane's guard is checked: a subsequent root-scoped assignment has no parent, so this returns without inspecting quarantined child lanes. Root-scoped commands can access .worktrees/* and include operations such as git worktree remove, allowing the quarantined lane to be changed or deleted before the required inspection/reset. Checkout admission needs to reject while any child-lane fence remains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36e7efd: a root-scoped assignment now checks every lane beneath its checkout, both the in-memory quarantine set and each lane's durable guard (the CLI lists .worktrees/*), before it runs.
| GIT_CONFIG_KEY_4: 'gc.auto', | ||
| GIT_CONFIG_VALUE_4: '0', | ||
| GIT_CONFIG_KEY_5: 'maintenance.auto', | ||
| GIT_CONFIG_VALUE_5: 'false', |
There was a problem hiding this comment.
These settings disable only automatic maintenance, while each concurrent lane still has write access to the same common Git object store. The inspected git gc -h describes --auto as “enable auto-gc mode,” and plain git gc, git gc --force, git repack, or git maintenance run remain executable; one lane can therefore prune or rewrite packs while a sibling is fetching or committing. Explicit shared-storage maintenance must be rejected or serialized with the checkout instead of relying on the auto-maintenance configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 36e7efd. Lanes can no longer write the common directory itself, only objects, refs, logs/refs, lfs and their own metadata, so git gc fails at gc.pid.lock and packed-refs. Checked with Git 2.34 under the same allowlist enforced by file modes. What remains is Git's documented concurrent-safe path: repack -d writes the new pack before removing old ones, and readers re-scan packs on a miss. Loose-object pruning stays behind gc.pruneExpire. An explicit git prune --expire=now is equivalent to running it in a second terminal, and the README says storage maintenance belongs in the checkout.
| /** Denied whether or not they exist yet, so a lane cannot create them for its siblings. */ | ||
| const readOnlyGitPaths = [ | ||
| ...LINKED_WORKTREE_PROTECTED_GIT_PATHS.map((path) => join(commonGitDir, path)), | ||
| ...siblings, |
There was a problem hiding this comment.
The deny list protects other linked-worktree metadata but omits the main checkout's per-worktree state, which Git stores directly in the common directory (for example .git/index, .git/HEAD, .git/logs/HEAD, and merge/rebase state). Because the lane sandbox grants writes to the entire common Git directory, a lane command that uses git rev-parse --git-common-dir can overwrite the parent checkout's staged files or branch state even though checkout and lane operations are intended to be isolated. Add the main worktree's private metadata paths to the read-only set while retaining writes only for genuinely shared objects and refs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36e7efd by switching from a deny list to a write allowlist. The lane can read .git but write only the shared objects, refs, logs/refs, lfs and its own worktrees/<name>. The checkout's HEAD, index, logs/HEAD and merge/rebase state are never writable. The file-mode simulation confirmed that writes to HEAD, index and MERGE_HEAD are refused while commit, merge, stash and rebase still work.
| private readonly executors = new Map< | ||
| string, | ||
| { fingerprint: string; value: Promise<LocalWorkspaceTools> } | ||
| >(); | ||
| private readonly commandRoots = new Map<string, string>(); |
There was a problem hiding this comment.
A long-lived worker that creates a fresh worktree name for each task permanently retains an entry in both of these maps after that worktree is removed; registering commands also leaves the corresponding ID in NativeWorkspaceCommandPool.roots, and credential routing retains the lane path. The child-process cache is bounded, but these registries are not, so normal task-per-worktree usage causes memory and registration state to grow with every historical lane until the worker restarts. Add lifecycle cleanup or a bounded idle eviction path for dynamic lanes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36e7efd: a lane is released when its verification fails, e.g. after git worktree remove. Release unregisters the pool root (skipped while it is busy), drops the executor and fires onRelease, which the CLI uses to drop the credential route. At most 32 lanes are kept, least recently used first.
…uarantined lanes, release stale lanes - A lane writes only shared objects, refs, ref logs, LFS storage and its own worktree metadata; the checkout HEAD, index, operation state, config and hooks stay read-only without per-path denies. - A checkout assignment waits on any quarantined lane beneath it. - Lane registrations are released when their worktree disappears and capped at 32, dropping command roots and credential routes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e7efd979
ℹ️ 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".
| const names = (await this.options.linkedWorktreeNames?.(workspaceId)) ?? []; | ||
| await Promise.all( | ||
| names.map(async (name) => (await resolveLaneGuard(workspaceId, name)).assertAvailable()), |
There was a problem hiding this comment.
Fresh evidence after the earlier root-fence fix is the cleanup ordering in executeOwned: a lane's local guard is cleared before reportWorkspaceOwnership(..., 'workspace-cleanup') is acknowledged. If that request fails, Redis retains the child fence and only the in-memory quarantinedWorkspaces entry blocks the checkout; after a worker restart, this check sees neither that entry nor the already-cleared local guard. A root-scoped command can then run and modify the quarantined lane or shared repository state, so retain durable child-fence state until the cleanup acknowledgment succeeds or consult a durable server-side child-fence index here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e98451a on the Code API side, so it survives worker restarts. Each lane enqueue adds its fence key to a durable index on the checkout fence (<checkout fence>:lanes). A checkout enqueues only after reserving a slot that conflicts with every lane beneath it, and a lane keeps its slot until cleanup is confirmed. So any indexed lane fence that still exists at checkout enqueue is stuck, and the checkout is refused with WORKSPACE_QUARANTINED. Members whose fences are gone are pruned in the same script. Covered in linked-worktree.test.ts.
| ], | ||
| allowRead: [ | ||
| root, | ||
| ...laneGitPaths, |
There was a problem hiding this comment.
This adds the external common Git directory only to the sandbox's base read policy. During selected-workspace PTC with replay tools, createProgrammaticProbeWorkspace clones the lane but preserves its absolute .git pointer, while the probe-specific filesystem override in executeProgrammatic allows reads only from the scratch clone and data directory. Consequently, code that runs a Git command before its first tool call fails during the probe; the worker sees no pending call, runs the commit pass, and then rejects the tool call as unexpected. Carry commonGitDir into the probe's read-only allowlist for linked lanes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e98451a: a lane's canonical common Git directory is added to the probe's allowRead (not allowWrite), so Git reads work in replay probes while the shared store stays read-only. Covered in native-sandbox.test.ts.
…bes read shared Git - Code API indexes the lane fences enqueued beneath each checkout. A checkout reaches enqueue only once no lane holds a slot, so an indexed fence that still exists is stuck and the checkout is refused, surviving worker restarts. - Replay probes of a lane may read its common Git directory (read-only).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e98451aab8
ℹ️ 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".
| * other path there (config, hooks, the checkout's own HEAD, index and | ||
| * operation state, sibling metadata) stays read-only. | ||
| */ | ||
| const LINKED_WORKTREE_SHARED_GIT_PATHS = ['objects', 'refs', join('logs', 'refs'), 'lfs']; |
There was a problem hiding this comment.
Fresh evidence after the earlier maintenance fix: with Git 2.43, git prune -h advertises --expire <time>, and with the common Git directory read-only except for exactly this allowlist, git prune --expire now exits successfully and deletes an unreachable object; git repack -ad also succeeds, with -d documented as removing redundant packs. Because sibling lanes execute concurrently, one lane can therefore prune an object another lane has written but not yet made reachable, corrupting its commit or fetch. Disabling automatic maintenance and blocking .git/gc.pid does not close this explicit-maintenance race, so these commands must be rejected or serialized with the checkout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, not fixed in this PR; this was the final review cycle. The remaining exposure is an explicit, agent-issued (or combined with it) in one lane racing a sibling that has written objects it has not yet made reachable. That is the same hazard as running prune from a second terminal, and routine operations are unaffected: cannot run in a lane, and automatic maintenance is off. It will be tracked as a follow-up. Options: reject destructive object-store commands in lanes, or serialize a lane that runs them with its checkout.
![lia-by-librechat[bot]](https://avatars.githubusercontent.com/in/4931833?s=60&v=4){kind=small}
There was a problem hiding this comment.
P1: Explicit Git maintenance can still delete a sibling lane's unpublished objects.
At b3143f1a2fc0c841e03b531653edd9a320c6523c, lane commands and replay programs can write throughout the same .git/objects tree. git prune --expire now does not need .git/gc.pid; in a disposable repository it removed an object written with git hash-object -w before any ref could reference it. A concurrent lane can be in exactly that window during commit or fetch. The unresolved Codex finding is correct. A top-level command-string filter would not cover scripts, aliases or hooks. Please exclude destructive maintenance from concurrent lanes with an enforceable sandbox policy, or serialize it with the checkout. The new README warning is not a fix. I would not enable these lanes until this is addressed.
I pushed b3143f1 to fix two other reproducible issues: symlinked shared Git storage could redirect a lane's write grant into sibling metadata, and an unresolved environment action bypassed the command wrapper. Both new regressions failed before the fix and pass afterward. Code-package typecheck and 67 focused tests pass. Service scheduling tests pass (9); its seven pre-existing type errors remain. Five real-Redis tests and the native sandbox test cannot run in this worker's network/filesystem environment, so CI must cover them.
2 participants
Summary
Agents that keep one registered checkout and give each task its own linked worktree (
git worktree add .worktrees/<task>) ran one at a time, even with free lease slots. The workspace isolation key was only the registered root (plus an optional conversation instance), so every call into any.worktrees/*folder of a repository shared one lane. Measured on a paired worker with 4 slots: two 15 s commands in one repository took 32.7 s, while three in three different repositories took 17.5 s.This adds linked-worktree lanes. A workspace-tool request (or programmatic call) may name
worktree: <name>. The worker verifies that<root>/.worktrees/<name>really is a linked worktree of that checkout and runs the request confined to it, withcwdand paths relative to the worktree. Code API schedules each lane as its own key nested beneath the checkout: sibling lanes run concurrently up to the negotiated slots, while a lane and its checkout exclude each other. The feature is opt-in on the worker (--linked-worktree-lanes) and negotiated in both directions, so existing workers, Code API deployments and callers behave exactly as before.Closes #268. Related: #267 (parallel-agent defaults), #257 (one broad root holding several repositories).
How it works
Keys and scheduling (Code API).
workspaceIsolationKey(workspaceId, instanceId?, worktree?)now produces\0linked-worktree\0<checkout key>\0<name>for a lane, andworkspaceIsolationParentrecovers the checkout key. The slot reservation script treats a key as conflicting with itself, its parent, and any busy lane beneath it:The last rule keeps root operations such as
git worktree addfrom being starved by a stream of lane work. At enqueue, a lane is refused withWORKSPACE_QUARANTINEDwhile its checkout's fence exists. A settled checkout keeps its slot until its cleanup is confirmed, so a waiting lane never sees a transient fence.Verification (worker), without running Git.
verifyLinkedWorktreerequires:.worktreesand.worktrees/<name>are real directories, with no symlink anywhere on the path;.gitis a regular file whosegitdirresolves to<root>/.git/worktrees/<name>;commondirresolves to<root>/.git, and itsgitdirpoints back to the worktree's.git.A plain directory, a borrowed
.gitpointer, a symlink alias, another repository's worktree placed under.worktrees, or a symlinked.worktreesare all rejected before dispatch.Confinement (worker). A lane's native sandbox is rooted at the worktree. It can read the checkout's common
.git, but it can write there only to shared storage (objects,refs,logs/refs,lfs) and to its ownworktrees/<name>metadata. Everything else stays read-only because it is never granted write access: configuration, hooks,info, the checkout's ownHEAD, index and merge/rebase state, and sibling metadata. Nothing is denied path by path, so nothing needs to exist in advance and no placeholder files appear in the checkout. Lane commands run withgc.auto=0andmaintenance.auto=false, andgit gcitself cannot run in a lane, since it must write.git/gc.pidandpacked-refs. The checkout is added as a Git safe directory. Checked with Git 2.34 against the same allowlist enforced by file modes, these work in a lane: commit, branch, checkout, merge, stash, rebase, tag,git repack -d, andgit maintenance run --task=commit-graph. Writes to the checkout'sHEAD, index,MERGE_HEAD, hooks and sibling metadata are refused.Scheduling, guards and wiring (worker).
.worktrees/*.LinkedWorktreeWorkspaceToolswraps the existing executors: requests without a worktree pass through unchanged, and lane roots are registered for GitHub App credential routing.workspaceScopes: ['git_linked_worktree']only when Code API returnssupportedWorkspaceScopes.--reset-workspace-worktree <name>added to the reset command.Scope limits.
git gc, must run from the checkout: Git takespacked-refs.lockin the common directory for every ref deletion.worktree, including a subdirectorycwdinto.worktrees/*, stay root-scoped.Rollout
workspaceScopesbefore any worker advertises it. The companion LibreChat PR does that and maps.worktrees/<name>/…paths onto the new field.--linked-worktree-lanes(requires native-srt commands and at least two lease slots).Testing
service:bun test ./srcwith a Redis 7 server: 1111 pass. The 5 failures (hashToolInputnumber spelling and native mutation handoff) also fail on unmodifiedmain.slots.test.tscases: siblings, checkout waiting, busy checkout, starvation guard, and lanes nested under a conversation checkout.linked-worktree.test.tsagainst real Redis: two sibling lanes leased on two slots at once while a checkout request queues, a lane admitted after its checkout's cleanup, a lane refused while its checkout is quarantined, and a lane refused without the negotiated scope.packages/code:npm test: no new failures; every failure also fails onmain(environment-dependent, such as missing Bash 5.2/jq for programmatic execution).linked-worktrees.test.ts, using real Git repositories: verification of a genuine worktree and each forgery listed above, lane file tools confined to the worktree, and command-root registration refreshed when siblings change.packages/codeis clean. Theservicetypecheck shows the same 7 pre-existing errors asmain..git/config,.git/hooksand a sibling worktree are refused.