You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/08/29 15:40:25 UTC
(magpie) branch main updated: fix(agent-isolation): make the gitignore safety check hook-safe (#1093)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie.git
The following commit(s) were added to refs/heads/main by this push:
new d5c56a4b fix(agent-isolation): make the gitignore safety check hook-safe (#1093)
d5c56a4b is described below
commit d5c56a4bf6fb9944e60094c375c5a4dc304975b6
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Sat Aug 29 17:40:18 2026 +0200
fix(agent-isolation): make the gitignore safety check hook-safe (#1093)
sandbox-add-project-root.sh refused to write settings.local.json on
every invocation from a git hook, reporting "is not gitignored" for a
file that was correctly ignored. Because the post-checkout hook is
exactly how the helper reaches a fresh worktree, the per-worktree
sandbox-allowlist entry that issue #197 asks for was never written
there -- reads under a new worktree kept hitting sandbox denials.
The check ran `cd "$(dirname "$file")" && git check-ignore "$file"`.
Git hooks export GIT_DIR, and with GIT_DIR set but no GIT_WORK_TREE
git treats the current directory as the work-tree root -- so after the
cd into .claude/, the root-anchored pattern
`/.claude/settings.local.json` was evaluated against .claude/ as the
root, where it cannot match.
Discovery now runs with GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE
stripped, and check-ignore runs from the resolved toplevel instead of
from a subdirectory.
Fixing that surfaced a second, older bug in the same block: the
fallback guard tested `[ -d "$(dirname "$file")/.." ]`, which is false
when .claude/ does not exist yet. On a first run the check therefore
matched neither branch and fell through to writing, so the safety
check protected nothing in precisely the case it was written for. The
replacement walks up to the nearest existing ancestor before asking
git anything, so it holds whether or not .claude/ is present.
Five regression tests cover both bugs; all five fail against the
previous script. They neutralise global and system git config,
including an explicit core.excludesFile override -- pointing
GIT_CONFIG_GLOBAL at an empty file is not enough, because git still
falls back to ~/.config/git/ignore, which commonly already ignores
settings.local.json and would mask the "refuses" cases.
---
tools/agent-isolation/sandbox-add-project-root.sh | 41 +++++--
.../tests/test_sandbox_add_project_root.py | 123 +++++++++++++++++++++
2 files changed, 155 insertions(+), 9 deletions(-)
diff --git a/tools/agent-isolation/sandbox-add-project-root.sh b/tools/agent-isolation/sandbox-add-project-root.sh
index be84bec0..5f5a9581 100755
--- a/tools/agent-isolation/sandbox-add-project-root.sh
+++ b/tools/agent-isolation/sandbox-add-project-root.sh
@@ -192,15 +192,38 @@ update_settings() {
# .gitignore — if we land here without that entry in place, the
# adopter setup is incomplete and the user should fix the
# .gitignore first.
- if ( cd "$(dirname "$file")" 2>/dev/null \
- && git check-ignore -q "$file" 2>/dev/null ); then
- : # ignored — safe to write
- elif [ -d "$(dirname "$file")/.." ] \
- && git -C "$(dirname "$file")" rev-parse --show-toplevel >/dev/null 2>&1; then
- # Inside a git repo and check-ignore returned non-zero (path
- # is not ignored). Refuse to write.
- warn "$file is not gitignored — refusing to write. Add /.claude/settings.local.json to the adopter's .gitignore and re-run."
- return 0
+ #
+ # Both git calls below run the repo discovery themselves, from the
+ # repo root, with the hook environment stripped. That matters: git
+ # hooks export GIT_DIR, and with GIT_DIR set but no GIT_WORK_TREE
+ # git treats the *current directory* as the work-tree root. Any
+ # check run from a subdirectory would then evaluate a root-anchored
+ # pattern like `/.claude/settings.local.json` against `.claude/` as
+ # the root, where it cannot match — so the check reported "not
+ # ignored" for a correctly-ignored file on every invocation from a
+ # hook (post-checkout, post-merge, …), and the helper refused to
+ # write. See the regression tests in
+ # tests/test_sandbox_add_project_root.py.
+ local git_env root probe
+ git_env="env -u GIT_DIR -u GIT_WORK_TREE -u GIT_INDEX_FILE"
+
+ # `.claude/` may not exist yet, so discover from the nearest
+ # existing ancestor rather than from the file's own parent.
+ probe=$(dirname "$file")
+ while [ ! -d "$probe" ] && [ "$probe" != "/" ] && [ "$probe" != "." ]; do
+ probe=$(dirname "$probe")
+ done
+
+ root=$($git_env git -C "$probe" rev-parse --show-toplevel 2>/dev/null) || root=""
+
+ if [ -n "$root" ]; then
+ if $git_env git -C "$root" check-ignore -q "$file" 2>/dev/null; then
+ : # ignored — safe to write
+ else
+ # Inside a git repo and the path is genuinely not ignored.
+ warn "$file is not gitignored — refusing to write. Add /.claude/settings.local.json to the adopter's .gitignore and re-run."
+ return 0
+ fi
fi
# If the parent dir is not in any git repo, we are running under a
# caller that already verified that. Allow the write to proceed.
diff --git a/tools/agent-isolation/tests/test_sandbox_add_project_root.py b/tools/agent-isolation/tests/test_sandbox_add_project_root.py
index 9e78c135..ace0d182 100644
--- a/tools/agent-isolation/tests/test_sandbox_add_project_root.py
+++ b/tools/agent-isolation/tests/test_sandbox_add_project_root.py
@@ -221,6 +221,129 @@ class TestNonGitDirectory:
assert not (non_git / ".claude" / "settings.local.json").exists()
+# ---------------------------------------------------------------------------
+# gitignore safety check
+# ---------------------------------------------------------------------------
+
+
+def _git_dir_of(repo: Path) -> str:
+ """Absolute .git dir, as a git hook would export it in GIT_DIR."""
+ return subprocess.run(
+ ["git", "-C", str(repo), "rev-parse", "--absolute-git-dir"],
+ check=True,
+ capture_output=True,
+ text=True,
+ ).stdout.strip()
+
+
+def _no_global_git_config(tmp_path: Path) -> dict:
+ """Env that makes only the repo's own .gitignore count.
+
+ A developer's global excludes commonly already ignore
+ `**/.claude/settings.local.json`, which would mask the "refuses"
+ cases and make these tests pass for the wrong reason on some
+ machines and not others.
+
+ Pointing ``GIT_CONFIG_GLOBAL`` at an empty file is not enough: when
+ ``core.excludesFile`` is unset git still falls back to its default
+ ``$XDG_CONFIG_HOME/git/ignore`` (``~/.config/git/ignore``). So the
+ stand-in global config has to set ``core.excludesFile`` explicitly.
+ """
+ cfg = tmp_path / "gitconfig-isolated"
+ cfg.write_text(f"[core]\n\texcludesFile = {os.devnull}\n")
+ return {"GIT_CONFIG_GLOBAL": str(cfg), "GIT_CONFIG_SYSTEM": os.devnull}
+
+
+def _bare_git_repo(tmp_path: Path) -> Path:
+ """An initialised repo with **no** .gitignore, so nothing is ignored."""
+ repo = tmp_path / "repo"
+ repo.mkdir()
+ subprocess.run(["git", "init", str(repo)], check=True, capture_output=True)
+ return repo
+
+
+class TestGitignoreSafetyCheck:
+ def test_refuses_when_not_gitignored(self, tmp_path: Path) -> None:
+ """A genuinely un-ignored settings file must not be written to.
+
+ Also covers the case where `.claude/` does not exist yet: the
+ check has to resolve the repo from the nearest existing ancestor
+ rather than from the file's own (missing) parent directory.
+ """
+ repo = _bare_git_repo(tmp_path)
+ result = _run(repo, extra_env=_no_global_git_config(tmp_path))
+ assert result.returncode == 0
+ assert "is not gitignored" in result.stderr
+ assert not (repo / ".claude" / "settings.local.json").exists()
+
+ def test_writes_when_git_dir_is_exported(self, tmp_path: Path) -> None:
+ """Regression: the check must survive the git-hook environment.
+
+ Git hooks export GIT_DIR. With GIT_DIR set and no GIT_WORK_TREE,
+ git treats the current directory as the work-tree root, so a
+ check run from a subdirectory evaluated the root-anchored
+ `/.claude/settings.local.json` pattern against `.claude/` as the
+ root and wrongly concluded the file was not ignored.
+ """
+ repo = _make_git_repo(tmp_path)
+ # `.claude/` must already exist, as it does in a real adopter
+ # repo. With it missing the pre-fix code failed its `cd` and
+ # fell through to writing anyway, which would mask the bug.
+ (repo / ".claude").mkdir()
+ result = _run(
+ repo, extra_env={**_no_global_git_config(tmp_path), "GIT_DIR": _git_dir_of(repo)}
+ )
+
+ assert "is not gitignored" not in result.stderr
+ assert result.returncode == 0
+ assert (repo / ".claude" / "settings.local.json").exists()
+
+ def test_git_dir_exported_still_records_repo_root(self, tmp_path: Path) -> None:
+ """The allowlist entry must be the repo root, not the .claude subdir."""
+ repo = _make_git_repo(tmp_path)
+ (repo / ".claude").mkdir()
+ _run(repo, extra_env={**_no_global_git_config(tmp_path), "GIT_DIR": _git_dir_of(repo)})
+ data = _load(repo / ".claude" / "settings.local.json")
+ assert str(repo) in data["sandbox"]["filesystem"]["allowRead"]
+
+ def test_git_dir_exported_still_refuses_when_not_gitignored(self, tmp_path: Path) -> None:
+ """The fix must not turn the safety check into a no-op."""
+ repo = _bare_git_repo(tmp_path)
+ result = _run(
+ repo, extra_env={**_no_global_git_config(tmp_path), "GIT_DIR": _git_dir_of(repo)}
+ )
+
+ assert "is not gitignored" in result.stderr
+ assert not (repo / ".claude" / "settings.local.json").exists()
+
+ def test_works_in_linked_worktree(self, tmp_path: Path) -> None:
+ """The real-world trigger: a hook firing inside a git worktree."""
+ repo = _make_git_repo(tmp_path)
+ subprocess.run(
+ ["git", "-C", str(repo), "add", ".gitignore"], check=True, capture_output=True
+ )
+ subprocess.run(
+ ["git", "-C", str(repo), "-c", "user.email=t@e.st", "-c", "user.name=t",
+ "-c", "commit.gpgsign=false", "commit", "-m", "init"],
+ check=True,
+ capture_output=True,
+ )
+ wt = tmp_path / "wt"
+ subprocess.run(
+ ["git", "-C", str(repo), "worktree", "add", "-q", str(wt), "-b", "wt"],
+ check=True,
+ capture_output=True,
+ )
+
+ (wt / ".claude").mkdir()
+ result = _run(wt, extra_env={**_no_global_git_config(tmp_path), "GIT_DIR": _git_dir_of(wt)})
+
+ assert "is not gitignored" not in result.stderr
+ assert (wt / ".claude" / "settings.local.json").exists()
+ data = _load(wt / ".claude" / "settings.local.json")
+ assert str(wt) in data["sandbox"]["filesystem"]["allowRead"]
+
+
# ---------------------------------------------------------------------------
# missing jq
# ---------------------------------------------------------------------------