mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-24 20:16:30 +00:00
fix(e2e): give the review sandbox a root node_modules can be hardlinked into
--with-deps hardlinks node_modules in, and a hardlink cannot cross filesystems, so on a host whose $TMPDIR is its own mount the flag was unusable: `cp -al` fails and the ERR trap tears the half-built worktree down, leaving the caller with an empty $SANDBOX. Reproduced on a real second filesystem (attached image, st_dev 16777238 vs 16777234): the old script dies with "Cross-device link" at rc=1, the new one probes with a real link first, reports the fallback on stderr and builds the sandbox beside the repo. Same-filesystem hosts keep using $TMPDIR untouched, and PENTAGI_SANDBOX_ROOT overrides both. `clean` accepts a path under either root, so a sandbox built through the fallback can still be removed by the containment check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
aa5378b9b2
commit
7fcecdc6b4
@@ -183,7 +183,10 @@ including the root itself; `clean --all` sweeps sandboxes untouched for two hour
|
||||
cannot take out a concurrent agent's live one. A bare `clean` is an error, not a sweep.
|
||||
|
||||
The sandbox is a `git worktree` under `$TMPDIR`, so a deleted file or an edited spec inside
|
||||
it cannot reach your tree. Two caveats: an absolute path still escapes it, and a tool that
|
||||
it cannot reach your tree. `--with-deps` needs the root to be on the repo's own filesystem —
|
||||
hardlinks cannot cross mounts — so where `$TMPDIR` is its own mount (tmpfs `/tmp`, a separate
|
||||
`/home`) the tool says so and falls back to `.pentagi-review-sandboxes` beside the repo;
|
||||
`PENTAGI_SANDBOX_ROOT` overrides both. Two caveats: an absolute path still escapes it, and a tool that
|
||||
rewrites a dependency **in place** would reach the shared inode — don't hand `--with-deps`
|
||||
to an agent whose job is patching libraries. Check `git status` in your own tree when a run
|
||||
finishes; that is the only proof nothing leaked.
|
||||
|
||||
@@ -21,10 +21,38 @@
|
||||
set -euo pipefail
|
||||
|
||||
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)"
|
||||
SANDBOX_ROOT="${TMPDIR:-/tmp}/pentagi-review-sandboxes"
|
||||
SANDBOX_ROOT="${PENTAGI_SANDBOX_ROOT:-${TMPDIR:-/tmp}}/pentagi-review-sandboxes"
|
||||
# A hardlink cannot cross filesystems, so --with-deps is unusable when $TMPDIR is its own mount
|
||||
# (tmpfs /tmp, a separate /home, a container volume): `cp -al` fails outright and no sandbox is
|
||||
# built. This sibling of the repo is on the repo's filesystem by construction.
|
||||
FALLBACK_ROOT="$(dirname "$REPO_ROOT")/.pentagi-review-sandboxes"
|
||||
|
||||
# Pick a root node_modules can actually be hardlinked into: probe with a real link, fall back to
|
||||
# the repo's own filesystem when the probe hits EXDEV.
|
||||
linkable_root() {
|
||||
local probe="$SANDBOX_ROOT/.linkprobe"
|
||||
|
||||
mkdir -p "$SANDBOX_ROOT"
|
||||
|
||||
if ln "${BASH_SOURCE[0]}" "$probe" 2>/dev/null; then
|
||||
rm -f "$probe"
|
||||
echo "$SANDBOX_ROOT"
|
||||
|
||||
return
|
||||
fi
|
||||
|
||||
if ! mkdir -p "$FALLBACK_ROOT" 2>/dev/null; then
|
||||
echo "review-sandbox: $SANDBOX_ROOT is on another filesystem than the repo and" \
|
||||
"$FALLBACK_ROOT is not writable — set PENTAGI_SANDBOX_ROOT to a same-filesystem path" >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
echo "review-sandbox: hardlinks cannot reach $SANDBOX_ROOT — using $FALLBACK_ROOT" >&2
|
||||
echo "$FALLBACK_ROOT"
|
||||
}
|
||||
|
||||
create() {
|
||||
local carry_dirty="" with_deps="" path
|
||||
local carry_dirty="" with_deps="" path root="$SANDBOX_ROOT"
|
||||
|
||||
for arg in "$@"; do
|
||||
case "$arg" in
|
||||
@@ -37,8 +65,12 @@ create() {
|
||||
esac
|
||||
done
|
||||
|
||||
path="$SANDBOX_ROOT/wt-$$-$(date +%s)"
|
||||
mkdir -p "$SANDBOX_ROOT"
|
||||
if [[ -n "$with_deps" ]]; then
|
||||
root="$(linkable_root)"
|
||||
fi
|
||||
|
||||
path="$root/wt-$$-$(date +%s)"
|
||||
mkdir -p "$root"
|
||||
git -C "$REPO_ROOT" worktree add --detach --quiet "$path" HEAD
|
||||
# Anything that fails from here leaves a worktree behind, and the caller reads stdout as the
|
||||
# sandbox path — so take the worktree down and say so on stderr rather than printing a stub.
|
||||
@@ -74,32 +106,48 @@ create() {
|
||||
# every agent on the host, so a bare `clean` used to take out a concurrent agent's live sandbox.
|
||||
STALE_MINUTES=120
|
||||
|
||||
# A strict child of either sandbox root — never a root itself, so a sibling like <root>-old and the
|
||||
# root's own path are both rejected.
|
||||
contained() {
|
||||
local candidate="$1" root
|
||||
|
||||
for root in "$SANDBOX_ROOT" "$FALLBACK_ROOT"; do
|
||||
mkdir -p "$root"
|
||||
|
||||
case "$candidate" in
|
||||
"$(cd "$root" && pwd -P)"/?*) return 0 ;;
|
||||
esac
|
||||
done
|
||||
|
||||
return 1
|
||||
}
|
||||
|
||||
clean() {
|
||||
local target="${1:-}" root resolved
|
||||
|
||||
case "$target" in
|
||||
--all)
|
||||
if [[ -d "$SANDBOX_ROOT" ]]; then
|
||||
find "$SANDBOX_ROOT" -maxdepth 1 -name 'wt-*' -mmin "+$STALE_MINUTES" -exec rm -rf {} +
|
||||
fi
|
||||
for root in "$SANDBOX_ROOT" "$FALLBACK_ROOT"; do
|
||||
if [[ -d "$root" ]]; then
|
||||
find "$root" -maxdepth 1 -name 'wt-*' -mmin "+$STALE_MINUTES" -exec rm -rf {} +
|
||||
fi
|
||||
done
|
||||
;;
|
||||
'')
|
||||
echo "review-sandbox: clean needs a sandbox path, or --all to sweep stale ones" >&2
|
||||
exit 2
|
||||
;;
|
||||
*)
|
||||
mkdir -p "$SANDBOX_ROOT"
|
||||
root="$(cd "$SANDBOX_ROOT" && pwd -P)"
|
||||
resolved="$(cd "$(dirname "$target")" 2>/dev/null && pwd -P || true)/$(basename "$target")"
|
||||
|
||||
case "$resolved" in
|
||||
*/. | */..) resolved="" ;;
|
||||
"$root"/?*) ;;
|
||||
*) resolved="" ;;
|
||||
*) contained "$resolved" || resolved="" ;;
|
||||
esac
|
||||
|
||||
if [[ -z "$resolved" ]]; then
|
||||
echo "review-sandbox: refusing to clean '$target' — not a sandbox under $root" >&2
|
||||
echo "review-sandbox: refusing to clean '$target' — not a sandbox under" \
|
||||
"$SANDBOX_ROOT or $FALLBACK_ROOT" >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
|
||||
@@ -74,6 +74,25 @@ describe('review-sandbox clean — containment', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('review-sandbox create — where the sandbox lands', () => {
|
||||
it('honours PENTAGI_SANDBOX_ROOT, the escape hatch for a $TMPDIR on another filesystem', () => {
|
||||
const override = mkdtempSync(join(tmpdir(), 'override-root-'));
|
||||
const sandbox = execFileSync(SCRIPT, ['create'], {
|
||||
encoding: 'utf8',
|
||||
env: { ...process.env, PENTAGI_SANDBOX_ROOT: override, TMPDIR: sandboxRoot },
|
||||
}).trim();
|
||||
|
||||
expect(sandbox.startsWith(`${override}/pentagi-review-sandboxes/`)).toBe(true);
|
||||
|
||||
execFileSync(SCRIPT, ['clean', sandbox], {
|
||||
env: { ...process.env, PENTAGI_SANDBOX_ROOT: override, TMPDIR: sandboxRoot },
|
||||
});
|
||||
expect(existsSync(sandbox)).toBe(false);
|
||||
|
||||
rmSync(override, { force: true, recursive: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('review-sandbox clean — the sweep', () => {
|
||||
it('refuses a bare clean rather than taking the whole root', () => {
|
||||
const { stdout: sandbox } = run('create');
|
||||
|
||||
Reference in New Issue
Block a user