From 3f3f686b4dbec05f17866f7dd10206a26960b448 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 17 Aug 2026 23:20:32 +0200 Subject: [PATCH] fix(agent-isolation): make the gitignore safety check hook-safe 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. --- .../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 be84bec02..5f5a95816 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 9e78c135d..ace0d182f 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 @@ def test_not_in_git_repo_creates_no_file(self, tmp_path: Path) -> None: 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 # ---------------------------------------------------------------------------