fix(dev): allow sandboxed npm to read the greptile tool store

.flox/env/on-activate.sh

@@ -374,27 +374,67 @@ fi
 # the venv bin (Step 2b), so worktrees on different branches resolve their
 # own pin. A failed install must not break activation; the CLI is only needed
 # at PR-open time and `hogli review` prints install guidance when absent.
+#
+# The store is an execute-later target: Step 2b links the binary into the venv
+# and `hogli review` runs it in the developer's shell, outside the sandbox. The
+# sandbox therefore write-denies the store (see bin/dev-sandbox.sb) and this
+# trusted, unsandboxed shell performs the install itself. Skipping the sandbox
+# is safe here: --ignore-scripts keeps package code from running at install
+# time (npm only downloads, integrity-checks, and extracts), and the CLI runs
+# unsandboxed at review time anyway. Do not sandbox the npm run instead: any
+# sandbox-writable path it installs through (a TMPDIR staging dir, the shared
+# ~/.npm cache) gives a concurrently sandboxed dependency a window to swap the
+# binary before it is published. Every input this npm run reads is pinned away
+# from sandboxed writers: the cache lives inside the write-denied store, the
+# user config is disabled (and the sandbox write-denies ~/.npmrc anyway), cwd
+# plus a fresh package.json keep the project-config lookup inside the store
+# instead of walking up to $HOME, and npm and flock themselves are resolved
+# into the immutable Nix store rather than taken from a PATH that contains
+# sandbox-writable dirs.
 _GREPTILE_VERSION="3.4.1"
 _GREPTILE_STORE="$HOME/.config/posthog/tools/greptile/$_GREPTILE_VERSION"
 _GREPTILE_BIN="$_GREPTILE_STORE/node_modules/.bin/greptile"
 _GREPTILE_STAMP="$_GREPTILE_STORE/.complete"
+_GREPTILE_CACHE="$_GREPTILE_STORE/.npm-cache"
 
 _install_greptile() {
   # Explicit `|| return`/`|| exit`: callers suppress errexit, so a failed
   # install would otherwise fall through and stamp the broken state.
+  #
+  # PATH here contains sandbox-writable dirs: the venv bin prepended above, and
+  # $FLOX_ENV/bin, which the repo's .flox/run symlink can be repointed at. A
+  # planted npm or flock shim there would run outside the sandbox. Resolve both
+  # through /usr/bin/readlink (a system binary, not PATH-resolved) and refuse
+  # anything that lands outside the immutable Nix store. The resolved npm's
+  # shebang is Nix-patched to the store node, so it does not look up node via
+  # PATH; the coreutils below come from the pinned system dirs.
+  local npm_bin flock_bin
+  npm_bin="$(/usr/bin/readlink -f "$(command -v npm)" 2>/dev/null)" || return 1
+  flock_bin="$(/usr/bin/readlink -f "$(command -v flock)" 2>/dev/null)" || return 1
+  [[ "$npm_bin" == /nix/store/* && "$flock_bin" == /nix/store/* ]] || return 1
+  local PATH="/usr/bin:/bin"
   mkdir -p "$_GREPTILE_STORE" || return 1
+  # A lock file planted as a symlink while the store was still sandbox-writable
+  # would make the `9>` redirection below truncate the symlink's target with
+  # the developer's own account. The write-deny keeps a new one from appearing.
+  if [[ -L "$_GREPTILE_STORE/.install.lock" ]]; then
+    rm -f "$_GREPTILE_STORE/.install.lock" || return 1
+  fi
   (
     # The store is shared across checkouts, so serialize concurrent
     # activations (fresh worktrees) installing the same version.
-    flock 9 || exit 1
+    "$flock_bin" 9 || exit 1
     if [[ ! -x "$_GREPTILE_BIN" || ! -f "$_GREPTILE_STAMP" ]]; then
-      if [[ "$_DEV_SANDBOX_INSTALLS" -eq 1 ]]; then
-        # printf %q: dev-sandbox re-parses its command string, so the path
-        # must survive a $HOME with spaces or quotes.
-        "$FLOX_ENV_PROJECT/bin/dev-sandbox" "npm install --prefix $(printf '%q' "$_GREPTILE_STORE") --no-fund --no-audit greptile@$_GREPTILE_VERSION" || exit 1
-      else
-        npm install --prefix "$_GREPTILE_STORE" --no-fund --no-audit "greptile@$_GREPTILE_VERSION" || exit 1
-      fi
+      cd "$_GREPTILE_STORE" || exit 1
+      # Rebuild from a clean slate: npm trusts an existing node_modules tree,
+      # and a store from before the write-deny can hold planted files, such as
+      # a package.json or .npmrc symlink the writes below would follow. Remove
+      # every entry except the lock this subshell holds open.
+      find . -mindepth 1 -maxdepth 1 ! -name .install.lock -exec rm -rf {} + || exit 1
+      echo '{}' > package.json || exit 1
+      NPM_CONFIG_USERCONFIG=/dev/null "$npm_bin" install --cache "$_GREPTILE_CACHE" \
+        --ignore-scripts --no-fund --no-audit "greptile@$_GREPTILE_VERSION" || exit 1
+      rm -rf "$_GREPTILE_CACHE"
       [[ -x "$_GREPTILE_BIN" ]] || exit 1
       touch "$_GREPTILE_STAMP" || exit 1
     fi

bin/dev-sandbox-selftest

@@ -187,6 +187,29 @@ check_write_blocked ".git/hooks/.dev_sandbox_selftest_probe" ".git/hooks"
 if [[ -d "$HOME/.cargo/bin" ]]; then
     check_write_blocked "$HOME/.cargo/bin/.dev_sandbox_selftest_probe" "cargo bin"
 fi
+# The machine-shared tool store: flox activation publishes the greptile CLI here
+# and hogli review executes it in the developer's shell, outside Seatbelt — the
+# same execute-later route as cargo bin above. Creating the dir is safe: this is
+# the store's real location, and activation creates it the same way.
+mkdir -p "$HOME/.config/posthog/tools" 2>/dev/null
+if [[ -d "$HOME/.config/posthog/tools" ]]; then
+    check_write_blocked "$HOME/.config/posthog/tools/.dev_sandbox_selftest_probe" "machine tool store (greptile runs unsandboxed)"
+    # A rename could swap in a symlinked ancestor and move the store out from
+    # under the subpath deny. chmod needs the same file-write* verdict as rename
+    # without touching the real directories, and the bits it sets are already set.
+    $SANDBOX "chmod u+rwx '$HOME/.config/posthog'" >/dev/null 2>&1
+    report_write_blocked $? "tool store parent node (\$HOME/.config/posthog rename swap)"
+    $SANDBOX "chmod u+rwx '$HOME/.config'" >/dev/null 2>&1
+    report_write_blocked $? "\$HOME/.config node (rename swap)"
+    # Positive control: the ancestor denies are literals, not subpaths — entries
+    # inside ~/.config must stay writable or sandboxed tools lose their config dirs.
+    if $SANDBOX "echo x > '$HOME/.config/.dev_sandbox_selftest_probe'" >/dev/null 2>&1; then
+        pass "write allowed: entry inside ~/.config (the deny is the node, not a subpath)"
+        rm -f "$HOME/.config/.dev_sandbox_selftest_probe"
+    else
+        die "write blocked, should be allowed: entry inside ~/.config"
+    fi
+fi
 # Homebrew's bin/sbin/Cellar: developer-owned and on PATH, same execute-later route
 # as cargo bin above — no git-config indirection needed to reach it.
 for hb_bin in /opt/homebrew/bin /usr/local/bin; do
@@ -213,6 +236,14 @@ if [[ -n "$gitcommon" && -f "$gitcommon/config" ]]; then
 else
     echo "  --   repo .git/config not found, skipping"
 fi
+# npm user config: registry/proxy/cafile keys redirect where an unsandboxed npm
+# (the activation's tool-store install) downloads code from — the ~/.gitconfig
+# escape with npm as the trusted tool.
+if [[ -f "$HOME/.npmrc" ]]; then
+    check_write_blocked_existing "$HOME/.npmrc" "npm user config (registry/proxy redirect)"
+else
+    check_write_blocked "$HOME/.npmrc" "npm user config (registry/proxy redirect)"
+fi
 # The scripts that actually run outside the sandbox. core.hooksPath points git at
 # .husky, so the .git/hooks deny above guards a directory git never reads once that
 # is set; these are the real hooks. .flox/env holds the activation scripts, and

bin/dev-sandbox.sb

@@ -156,6 +156,11 @@
     (literal (string-append (param "HOME") "/.gitconfig"))        ; core.hooksPath / credential.helper = !cmd
     (literal (string-append (param "HOME") "/.gitconfig.local"))  ; included by ~/.gitconfig — same escape if writable
     (subpath (string-append (param "HOME") "/.config/git"))       ; XDG git config — same hooksPath/helper escape
+    ;; npm user config. Its registry, proxy, and cafile keys redirect where an
+    ;; unsandboxed npm (such as the activation's tool-store install above)
+    ;; downloads code from, which is the ~/.gitconfig escape with npm as the
+    ;; trusted tool. Reads were already denied with the rest of $HOME.
+    (literal (string-append (param "HOME") "/.npmrc"))
     ;; Homebrew's system-scope gitconfig, read by a brew-installed git on every
     ;; invocation and owned by the developer's account — unlike the Xcode CLT one,
     ;; which is root-owned and so already out of reach. Same hooksPath/fsmonitor
@@ -183,7 +188,23 @@
     (subpath "/usr/local/sbin")
     (subpath "/usr/local/Cellar")
     (subpath "/usr/local/lib")
-    (subpath (string-append (param "HOME") "/.cargo/bin")))       ; binaries on PATH
+    (subpath (string-append (param "HOME") "/.cargo/bin"))        ; binaries on PATH
+    ;; The machine-shared tool store. The unsandboxed flox activation installs the
+    ;; greptile CLI here (npm --ignore-scripts, so no package code runs), links it
+    ;; into the venv bin, and `hogli review` executes it in the developer's shell —
+    ;; the same execute-later route as cargo bin above. The venv bin symlink itself
+    ;; cannot be denied here: sandboxed `uv sync` creates and populates the venv, so
+    ;; a sandboxed dependency can replace that link. `hogli review` therefore refuses
+    ;; a greptile that resolves outside the write-denied locations (see
+    ;; _TRUSTED_INSTALL_ROOTS in tools/hogli-commands/hogli_commands/review.py);
+    ;; this deny is what makes the store binary it does run trustworthy.
+    (subpath (string-append (param "HOME") "/.config/posthog/tools"))
+    ;; …and the ancestor nodes, so a rename cannot swap in a symlinked ~/.config or
+    ;; ~/.config/posthog and move the store out from under the subpath deny — the
+    ;; same node-swap route as the /opt/homebrew/etc denies above. Entries inside
+    ;; ~/.config are their own paths and stay writable.
+    (literal (string-append (param "HOME") "/.config/posthog"))
+    (literal (string-append (param "HOME") "/.config")))
 
 ;; husky's installer writes .husky/_/ (a gitignored husky.sh plus a .gitignore) on
 ;; every sandboxed `pnpm install`, and it throws when that write fails, which would

tools/hogli-commands/hogli_commands/review.py

@@ -30,8 +30,11 @@
 
 from __future__ import annotations
 
+import os
+import sys
 import shutil
 import subprocess
+from pathlib import Path
 
 import click
 from hogli.manifest import REPO_ROOT
@@ -44,6 +47,49 @@
     "Re-enter the flox environment (activation installs it), "
     "or install it with `brew install greptileai/tap/greptile` or `npm install -g greptile`, then re-run."
 )
+
+# PATH in an activated shell starts with the venv bin, which the dev sandbox
+# can write to (the sandboxed `uv sync` populates it), so a compromised
+# dependency could plant a `greptile` there that would run unsandboxed with
+# the developer's account. Only execute a binary that resolves into a location
+# bin/dev-sandbox.sb write-denies: the PostHog tool store, the Homebrew
+# prefixes, or the immutable Nix store. Keep this list in sync with the
+# `deny file-write*` block in bin/dev-sandbox.sb.
+_TRUSTED_INSTALL_ROOTS: tuple[Path, ...] = (
+    Path.home() / ".config" / "posthog" / "tools",
+    Path("/opt/homebrew/bin"),
+    Path("/opt/homebrew/sbin"),
+    Path("/opt/homebrew/Cellar"),
+    Path("/opt/homebrew/lib"),
+    Path("/usr/local/bin"),
+    Path("/usr/local/sbin"),
+    Path("/usr/local/Cellar"),
+    Path("/usr/local/lib"),
+    Path("/nix/store"),
+)
+
+# greptile's shebang is `#!/usr/bin/env node`, and it spawns git, so its
+# subprocesses resolve executables from PATH at execution time. Give them a
+# PATH of write-denied and system dirs only, so a binary planted in the
+# sandbox-writable venv bin (first on the activated shell's PATH) cannot run.
+_TRUSTED_PATH_DIRS = (
+    "/opt/homebrew/bin",
+    "/usr/local/bin",
+    "/usr/bin",
+    "/bin",
+    "/usr/sbin",
+    "/sbin",
+)
+
+
+def _trusted_location(path: str) -> Path | None:
+    """The canonical path when it sits in a write-denied install location, else None."""
+    resolved = Path(path).resolve()
+    if any(resolved.is_relative_to(root) for root in _TRUSTED_INSTALL_ROOTS):
+        return resolved
+    return None
+
+
 _SIGNIN_HINT = (
     "Run `greptile login`, or set GREPTILE_API_KEY in .env.local (see .env.local.example). "
     "No access? The reviewing-before-pr skill has a harness-review fallback."
@@ -64,35 +110,37 @@
 _CHECK_COMMIT_LIMIT = 20
 
 
-def _probe(cmd: list[str]) -> subprocess.CompletedProcess[str] | None:
+def _probe(cmd: list[str], env: dict[str, str] | None) -> subprocess.CompletedProcess[str] | None:
     try:
-        return subprocess.run(cmd, capture_output=True, text=True, timeout=_PROBE_TIMEOUT_SECONDS)
+        return subprocess.run(cmd, capture_output=True, text=True, timeout=_PROBE_TIMEOUT_SECONDS, env=env)
     except (OSError, subprocess.SubprocessError):
         return None
 
 
-def _signed_in(binary: str) -> bool:
+def _signed_in(binary: str, env: dict[str, str] | None) -> bool:
     """False only on Greptile's explicit signed-out error; any other ``config``
     failure falls through to the review call, which reports the real problem.
     The string match is forced: greptile exits 1 for signed-out and for
     ordinary failures alike."""
-    result = _probe([binary, "config"])
+    result = _probe([binary, "config"], env)
     return result is None or result.returncode == 0 or "not signed in" not in (result.stdout + result.stderr).lower()
 
 
-def _branch_commits(base: str | None) -> list[str]:
+def _branch_commits(base: str | None, env: dict[str, str] | None) -> list[str]:
     # Match change_detection's base convention: origin/master, then master for
     # clones without the remote ref.
     for ref in [base] if base is not None else ["origin/master", "master"]:
-        result = _probe(["git", "-C", str(REPO_ROOT), "rev-list", f"--max-count={_CHECK_COMMIT_LIMIT}", f"{ref}..HEAD"])
+        result = _probe(
+            ["git", "-C", str(REPO_ROOT), "rev-list", f"--max-count={_CHECK_COMMIT_LIMIT}", f"{ref}..HEAD"], env
+        )
         if result is not None and result.returncode == 0:
             return result.stdout.split() or ["HEAD"]
     return ["HEAD"]
 
 
-def check(binary: str, base: str | None) -> int:
-    for commit in _branch_commits(base):
-        status = _probe([binary, "review", "status", "--commit", commit])
+def check(binary: str, base: str | None, env: dict[str, str] | None) -> int:
+    for commit in _branch_commits(base, env):
+        status = _probe([binary, "review", "status", "--commit", commit], env)
         if status is None:
             # A hung or broken probe would hang or break for every commit too.
             break
@@ -112,14 +160,44 @@ def run(branch: str | None, instructions: str | None, force: bool, do_check: boo
     if binary is None:
         click.secho(f"Greptile CLI not found. {_INSTALL_HINT}", fg="red", err=True)
         return 1
-    if not _signed_in(binary):
+    env: dict[str, str] | None = None
+    # The dev sandbox only exists on macOS; elsewhere these checks would just
+    # break legitimate installs (an npm prefix under $HOME) for no protection.
+    if sys.platform == "darwin":
+        trusted = _trusted_location(binary)
+        if trusted is None:
+            click.secho(
+                f"Not running greptile from {Path(binary).resolve()}: only write-protected install locations "
+                "are trusted (the PostHog tool store, Homebrew, or the Nix store). "
+                "Re-enter the flox environment to reinstall it, or use `brew install greptileai/tap/greptile`, "
+                "then re-run.",
+                fg="red",
+                err=True,
+            )
+            return 1
+        # Execute the canonical path, not the PATH entry which() found: that
+        # entry is usually the sandbox-writable venv symlink, which a sandboxed
+        # process could repoint between this validation and the exec.
+        binary = str(trusted)
+        node = shutil.which("node")
+        node_dir = None if node is None else _trusted_location(node)
+        if node_dir is None:
+            click.secho(
+                "node was not found in a write-protected location, and greptile runs on node. "
+                "Re-enter the flox environment, then re-run.",
+                fg="red",
+                err=True,
+            )
+            return 1
+        env = {**os.environ, "PATH": ":".join([str(node_dir.parent), *_TRUSTED_PATH_DIRS])}
+    if not _signed_in(binary, env):
         click.secho(f"Not signed in to Greptile. {_SIGNIN_HINT}", fg="yellow", err=True)
         return EX_CONFIG
     if do_check:
-        return check(binary, branch)
+        return check(binary, branch, env)
 
     if not force:
-        status = _probe([binary, "review", "status", "--commit", "HEAD"])
+        status = _probe([binary, "review", "status", "--commit", "HEAD"], env)
         if status is not None and status.returncode == _STATUS_COMPLETED:
             click.secho(
                 "HEAD already has a completed review. Showing it. Pass --force to start a new one.",
@@ -130,15 +208,15 @@ def run(branch: str | None, instructions: str | None, force: bool, do_check: boo
             return 0
         if status is not None and status.returncode == _STATUS_RUNNING:
             click.secho("A review for this branch is still running. Resuming it.", fg="cyan", err=True)
-            return subprocess.run([binary, "review", "--resume"]).returncode
+            return subprocess.run([binary, "review", "--resume"], env=env).returncode
 
     cmd = [binary, "review"]
     if branch is not None:
         cmd += ["--branch", branch]
     if instructions is not None:
         cmd += ["--instructions", instructions]
     # Inherit stdio so Greptile's own progress and interactive review view work.
-    return subprocess.run(cmd).returncode
+    return subprocess.run(cmd, env=env).returncode
 
 
 @click.command(