🛡️ fix: Guard Linked-Worktree Lanes Against Accidental Git Pruning - #272
Conversation
…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.
…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.
…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).
…it-guard # Conflicts: # packages/code/README.md # packages/code/src/cli.ts # packages/code/src/native-sandbox.test.ts # packages/code/src/native-sandbox.ts
|
@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: dc191605b3
ℹ️ 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".
|
@codex review the latest head |
![lia-by-librechat[bot]](https://avatars.githubusercontent.com/in/4931833?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed and fixed both Codex findings at 4242d448fb6e2db60c8d822fbdcef840db15d8d1.
- P1, Git aliases: A real linked worktree sharing its checkout's
.git/configreproduced the destructive alias path. The lane wrapper now checks aliases with the real Git executable under the same parsed-C,--git-dir,-c, include and--config-envconfiguration before forwarding any subcommand. All configured aliases are deliberately refused, including harmless, nested and shell aliases; parsing their bodies is not safe. Config-query errors fail closed. Ordinary Git commands still work. - P2, pathspec globals: The wrapper now accepts
--literal-pathspecs,--glob-pathspecs,--noglob-pathspecsand--icase-pathspecs. A real Git test stages a filename containing brackets with--literal-pathspecs; adding a pathspec flag cannot bypass destructive-command rejection.
Both new tests failed on the old head and pass now. Code-package npx tsc --noEmit, 78 focused guard/linked-worktree/pool/process/protocol/worker tests, the CLI gating test, the Windows fail-closed test and generated Bash syntax validation passed. The wrapper's lane-only PATH mount, read-only policy, cleanup, config scope, object preservation, and checkout/root behavior were reviewed. The guardrail is not a security boundary: an absolute Git path, changed PATH or caller shell alias can bypass it, as the README states.
At posting, exact-head CI is running. Local native-sandbox integration tests cannot execute on this worker because its / owner fails the private-storage preflight; CI covers them. Service tests/typecheck (service unchanged), full suites, ShellCheck and code-package lint/import-sort (not installed/configured) were not run locally. A maintainer must trigger a fresh Codex review; the GitHub App cannot.
|
CI update for exact head The process-lock test passed three isolated local Node 24 runs and five additional parallel runs alongside the changed Git-guard tests. GitHub declined a targeted job rerun, both while the workflow was running and after it finished ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4242d448fb
ℹ️ 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".
There was a problem hiding this comment.
Reviewed all three new Codex P1 findings at f9c6131185ab72bb0ccde158a51c91e00caa6dd6. They were fresh, reproducible gaps in the ordinary Git command guardrail, not stale comments.
- Reject
git lfs fetch/pull --pruneand-pbefore calling Git. Git LFS is not installed on this worker, so local tests prove flag interception rather than a live LFS deletion. - Reject
fetch/pull --auto-maintenanceand--auto-gc; append-c maintenance.auto=false -c gc.auto=0after caller global options. Tests verify-c,--config-envandGIT_CONFIG_COUNTcannot override the wrapper's effective values; a normalfetch --no-auto-maintenancestill works. - Append
-c help.autocorrect=0last sogit prun --expire nowdoes not autocorrect into an unguarded prune. The disposable-repository regression proves an unpublished object survives both repository-config and caller-cautocorrection.
The three regressions failed before the fix and pass afterward. Code-package tsc --noEmit, 81 focused Git/linked-worktree/pool/process/protocol/worker tests, one CLI gating test, one Windows fail-closed test and generated Bash syntax validation passed. Exact-head CI: all 10 checks successful. No service code changed; local service tests/typecheck, native-sandbox tests (this worker's / ownership blocks its private-storage preflight), ShellCheck (not installed), full suites and code-package lint/import-sort (not configured) were not run.
Scope remains a guardrail, not a sandbox boundary. Absolute Git paths, custom scripts and a changed PATH can still bypass it. To guarantee no concurrent object pruning by arbitrary lane shell code would require a different isolation or serialization design. I reviewed the entry points, config precedence, alias expansion and error behavior as one subsystem rather than treating a passing wrapper test as proof of a hard boundary.
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9c6131185
ℹ️ 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".
There was a problem hiding this comment.
Rechecked PR #272 at c6e0703836bf16b9c4d90cb156f0fa3bde53a21e. The new Codex P1 was fresh and reproducible: git for-each-repo --config=maintenance.repo prune --expire now deleted an unpublished object in a disposable repository without re-entering the lane PATH wrapper. The worker now refuses all lane for-each-repo calls, including harmless ones, because it cannot inspect Git's internally dispatched commands. Checkout/root commands are unaffected.
The regression failed before this commit and passes now; an orphaned object survives every denied invocation. Local checks passed: code-package npx tsc --noEmit, 61 focused guard/lane/process/pool/worker tests (including seven real-Git guard tests), and two CLI/Windows fail-closed tests. Exact-head CI is running. Full suites, live LFS tests (binary unavailable), native sandbox integration tests (worker host ownership blocks private-storage preflight), service typecheck/tests (service unchanged), code-package lint/import-sort (not configured) and a fresh Codex review (maintainer trigger needed) were not run locally.
This remains a guardrail against accidental commands, not an enforceable shell-code boundary: an absolute Git path or script changing PATH can still bypass it. A hard guarantee against concurrent pruning would require serialized lane commands or an isolated object store.
2 participants
Summary
Linked-worktree lanes in #270 share
.git/objects. A lane runninggit prune --expire nowcan delete an object a sibling has written but not yet referenced. This adds a lane-only, read-onlygitwrapper first on the sandbox commandPATH. It parses common Git global options, refusesprune,gc,repack,prune-packed,maintenance,multi-pack-index, andgit lfs prune, and forwards other commands to an operator-installed Git executable by absolute path.The wrapper lives in a worker-private sibling of writable sandbox scratch, is explicitly readable and denied writes in ordinary and replay-probe policies, and is removed on close or failed initialization. Checkout commands are unchanged. Windows lane opt-in fails closed because this wrapper requires POSIX.
Scope and dependency
This is a guardrail against accidental maintenance, not a security boundary. An absolute path to Git, a rewritten
PATH, or a script that does either bypasses it. Agents that intentionally bypass the wrapper can still trigger the shared-object data-loss race. Use lanes only where that limitation is acceptable.This follow-up depends on #270. It targets
main; until #270 lands, GitHub shows its parent commits in this PR. The new change alone isb3143f1..51b7c86. Do not merge it before #270. The original PR branch and review state are unchanged.Verification
cd packages/code && npx tsc --noEmit: passed./ownership fails the existing private-storage preflight. CI must run them.