diff --git a/docs/labels-and-capabilities.md b/docs/labels-and-capabilities.md index 1ae5ce09c..9a533d2aa 100644 --- a/docs/labels-and-capabilities.md +++ b/docs/labels-and-capabilities.md @@ -389,7 +389,7 @@ or a contract-free mix of substrates (e.g. `tools/spec-inventory` is | [`tools/skill-reconciler-diff`](../tools/skill-reconciler-diff/) | `substrate:framework-dev` | Deterministic structural diff between two skill trees — parses frontmatter, section headings, step inventory, placeholders, support files, and safety-baseline clauses into a JSON diff object for the `skill-reconciler` skill | | [`tools/vcs`](../tools/vcs/) | `contract:source-control` | Backend-dispatching implementation of the source-control (VCS) capability ([`tools/github/source-control.md`](../tools/github/source-control.md)); complete Git, Mercurial (Hg), Fossil, and Subversion (SVN) backends | | [`tools/sourcehut`](../tools/sourcehut/) | `contract:tracker` + `contract:source-control` + `contract:mail-archive` | SourceHut (sr.ht) forge bridge: todo.sr.ht, lists.sr.ht, builds.sr.ht, and git/hg repository reads | -| [`tools/hook-manager`](../tools/hook-manager/) | `contract:hook-manager` | Contract for git-hook managers under the secure agent setup: the install-target and home rules, and the registry of adapters (prek and overcommit) that the hook-manager shim, the whole-user dispatcher and `sandbox-lint` are generated from | +| [`tools/hook-manager`](../tools/hook-manager/) | `contract:hook-manager` | Contract for git-hook managers under the secure agent setup: the install-target and home rules, and the registry of adapters (prek, overcommit and lefthook today) that the hook-manager shim, the whole-user dispatcher and `sandbox-lint` are generated from | | [`tools/tracker`](../tools/tracker/) | `contract:tracker` | Interface spec for the tracker contract's read-only contributor-activity queries (issues filed, triaged and commented on; issues created; first maintainer reply). Pure interface spec; GitHub and Jira implement it, and the rest of the contract is described by `tools/github/tool.md` | | [`tools/body-field-contract`](../tools/body-field-contract/) | `contract:body-field` | The body-field contract: how every reader and writer finds a tracker body's `### ` fields, with a language-neutral conformance suite run against every implementation. Pure interface spec; implementations: `tools/github-body-field/` (reference), `tools/vetted-ops/`, `tools/local-tracker/`, `tools/jira/` | | [`tools/security-tracker`](../tools/security-tracker/) | `contract:security-tracker` | Adapter contract for the security tracker the `security-*` skills read and write, selected by `tracker.platform`. Pure interface spec; backends: `tools/github/` (default), `tools/local-tracker/`, `tools/jira/` | diff --git a/docs/setup/sandbox-troubleshooting.md b/docs/setup/sandbox-troubleshooting.md index a94d08a2b..308fc23c2 100644 --- a/docs/setup/sandbox-troubleshooting.md +++ b/docs/setup/sandbox-troubleshooting.md @@ -61,19 +61,24 @@ - [Root cause](#root-cause-10) - [Fix](#fix-10) - [Notes](#notes-9) - - [Reads of `~/.claude/magpie` or `/tmp/claude-` ask for approval every time](#reads-of-claudemagpie-or-tmpclaude-uid-ask-for-approval-every-time) + - [`lefthook install` refuses with "Custom hooks paths are not supported by default"](#lefthook-install-refuses-with-custom-hooks-paths-are-not-supported-by-default) - [Symptom](#symptom-11) - [Root cause](#root-cause-11) - [Fix](#fix-11) - [Notes](#notes-10) - - [Every Bash call fails with `bwrap: Can't create file … Read-only file system`](#every-bash-call-fails-with-bwrap-cant-create-file--read-only-file-system) + - [Reads of `~/.claude/magpie` or `/tmp/claude-` ask for approval every time](#reads-of-claudemagpie-or-tmpclaude-uid-ask-for-approval-every-time) - [Symptom](#symptom-12) - [Root cause](#root-cause-12) - [Fix](#fix-12) - - [`git status` shows files you never created, such as `.mcp.json` or `.bash_profile`](#git-status-shows-files-you-never-created-such-as-mcpjson-or-bash_profile) + - [Notes](#notes-11) + - [Every Bash call fails with `bwrap: Can't create file … Read-only file system`](#every-bash-call-fails-with-bwrap-cant-create-file--read-only-file-system) - [Symptom](#symptom-13) - [Root cause](#root-cause-13) - [Fix](#fix-13) + - [`git status` shows files you never created, such as `.mcp.json` or `.bash_profile`](#git-status-shows-files-you-never-created-such-as-mcpjson-or-bash_profile) + - [Symptom](#symptom-14) + - [Root cause](#root-cause-14) + - [Fix](#fix-14) - [Adding a new entry](#adding-a-new-entry) @@ -1236,6 +1241,45 @@ resolved path. Confirm with `git hook run pre-commit` from the agent. --- +## `lefthook install` refuses with "Custom hooks paths are not supported by default" + +### Symptom + +A repository that uses lefthook has no working hooks, and `lefthook install` fails: + +```console +$ lefthook install +core.hooksPath is set globally to '/Users//.claude/git-hooks' +Custom hooks paths are not supported by default. +... +``` + +### Root cause + +Whole-user scope sets a global `core.hooksPath`, and lefthook refuses to install while a scoped `git config --global` read returns one. +That is the safe failure: the alternative, `--force`, writes lefthook's hook into the shared directory and replaces the framework's dispatcher for every repository on the host. + +### Fix + +Run `lefthook install` through the framework shim, which hides the global config from that read and leaves the hook in the repository's own `$(git rev-parse --git-common-dir)/hooks/`: + +```bash +~/.claude/bin/lefthook install +``` + +With `~/.claude/bin` ahead of the real binary on PATH, plain `lefthook install` does that already. +The setup installs the shim only for a repository that uses lefthook: a root `lefthook.*`, `.lefthook.*` or `-local` config, or an installed hook mentioning `call_lefthook`. + +### Notes + +- Do not run `lefthook install --reset-hooks-path` or `--force` to get past the error: the first unsets the whole-user setup, and the second overwrites the dispatcher for every repository. + The agent-guard `lefthook-hooks-path` guard blocks both, in the `--flag` and `--flag=true` spellings, and needs `MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1` to proceed deliberately. +- An npm `postinstall` runs `node_modules/.bin/lefthook`, which the shim does not cover; that install refuses for the same reason, and the repository needs one run through the shim. +- The shim hides the global config only while it holds a `core.hooksPath`, so on a host without one the install keeps the operator's `credential.helper`, `url.*.insteadOf` and `safe.directory`. +- Per-project scope has no global `core.hooksPath`, so the refusal does not happen there. + +--- + ## Reads of `~/.claude/magpie` or `/tmp/claude-` ask for approval every time ### Symptom diff --git a/docs/setup/secure-agent-setup.md b/docs/setup/secure-agent-setup.md index d96537ee6..764300620 100644 --- a/docs/setup/secure-agent-setup.md +++ b/docs/setup/secure-agent-setup.md @@ -1442,10 +1442,13 @@ a no-op on hosts with no global `core.hooksPath`. When the dispatcher chains into an overcommit hook, it exports the same `core.hooksPath` override so overcommit's run-time self-check accepts the hook instead of looking for its hook in the shared dir. +lefthook refuses to install at all while a global `core.hooksPath` is set, so the same shim installed as `~/.claude/bin/lefthook` runs `lefthook install` with `GIT_CONFIG_GLOBAL=/dev/null` whenever the global config holds one, and lefthook then resolves its own target to the common dir. +`lefthook install --force` and `--reset-hooks-path` would still get that override through the shim (they cannot reach the shared dir or unset the setup), and the agent-guard `lefthook-hooks-path` guard blocks an agent from trying. +The shim covers the CLI only: an npm `postinstall` runs `node_modules/.bin/lefthook`, ahead of `~/.claude/bin` on PATH. The shim, the host-only homes and the sandbox-lint checks all follow the [hook-manager contract](../../tools/hook-manager/README.md): one script serves every adapter in its registry, keyed by the name it is -installed under, and other hook managers (pre-commit, lefthook, husky, …) +installed under, and other hook managers (pre-commit, husky, …) join by adding an adapter there. Validated behaviour under this flavour (`core.hooksPath` global + diff --git a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py index cc847e17f..7d9077663 100644 --- a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py +++ b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py @@ -2337,8 +2337,9 @@ def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str # Only commands in these families are inspected; everything else takes the # instant fast path. Bundled and contributed guards alike operate on the -# `gh` / `git` outbound/destructive surface. -GUARDED_HEADS = frozenset({"gh", "git"}) +# `gh` / `git` outbound/destructive surface and on `lefthook`, whose +# `install` can undo the whole-user hook setup. +GUARDED_HEADS = frozenset({"gh", "git", "lefthook"}) # Colon-separated extra guard directories, searched before the discovered ones. # Skill-owned guards no longer need it (``framework_root`` finds them in place); @@ -2597,12 +2598,32 @@ def run(self, args: list[str]) -> str | None: return _run(args, cwd=self.cwd) +def guarded_head(argv: list[str]) -> str | None: + """The guarded command family a segment runs, or None. + + The head is usually the bare name, but a manager installed from a package + tree is reached by path (``node_modules/.bin/lefthook``), so the basename + counts too. + """ + if not argv: + return None + head = argv[0] + if head in GUARDED_HEADS: + return head + base = head.replace("\\", "/").rsplit("/", 1)[-1] + if base.lower().endswith(".exe"): + base = base[:-4] + return base if base in GUARDED_HEADS else None + + def command_kinds(seg: Segment) -> set[str]: """The command-family tags a segment matches, e.g. ``{"git", "git:commit"}``.""" kinds: set[str] = set() if not seg.argv: return kinds - head = seg.argv[0] + head = guarded_head(seg.argv) + if head is None: + return kinds kinds.add(head) if head == "git": # A TRIGGERS = ["git:commit"] contributed guard must still fire behind @@ -2615,6 +2636,11 @@ def command_kinds(seg: Segment) -> set[str]: # Same for TRIGGERS = ["gh:pr"] behind `gh -R pr ...`. sub = gh_subcommand(seg.argv) kinds.add(f"{head}:{sub[0] if sub else seg.argv[1]}") + elif head == "lefthook": + # lefthook accepts global flags before the subcommand. + lefthook_sub = next((tok for tok in seg.argv[1:] if not tok.startswith("-")), None) + if lefthook_sub: + kinds.add(f"lefthook:{lefthook_sub}") return kinds @@ -2782,7 +2808,7 @@ def dispatch(command: str, cwd: str | None = None, harness: str | None = None) - if reason: return reason continue - if not seg.argv or seg.argv[0] not in GUARDED_HEADS: + if not seg.argv or guarded_head(seg.argv) is None: continue # fast path — non-guarded command family # Bundled guards (trusted, in-process; each self-filters its command). for builtin in BUILTIN_GUARDS: diff --git a/plugins/magpie-setup/skills/isolated-setup-install/guards/lefthook_hooks_path.py b/plugins/magpie-setup/skills/isolated-setup-install/guards/lefthook_hooks_path.py new file mode 100644 index 000000000..6a706ce9c --- /dev/null +++ b/plugins/magpie-setup/skills/isolated-setup-install/guards/lefthook_hooks_path.py @@ -0,0 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""setup-isolated-setup-install lefthook hooks-path guard (skill-contributed). + +`lefthook install --force` writes lefthook's hook into the directory the global +`core.hooksPath` names, over the framework's dispatcher, and +`--reset-hooks-path` unsets the whole-user setup. Both are destructive when +that path is the framework's own. Discovered by the agent-guard dispatcher; +import-free (uses ``ctx``). +""" + +import os + +TRIGGERS = ["lefthook:install"] + +FRAMEWORK_HOOKS_PATHS = ("~/.claude/git-hooks", "~/.claude-config/git-hooks") + + +def _install_arguments(argv): + """The arguments after the ``install`` subcommand, or an empty list.""" + for index, token in enumerate(argv[1:], start=1): + if token.startswith("-"): + continue + return argv[index + 1 :] if token == "install" else [] + return [] + + +def _destructive_flag(argument): + # lefthook takes the `--flag=value` spelling for its boolean flags, so match + # the name and read the value separately: `--force=true` is destructive, + # `--force=false` is not. + name, _, value = argument.partition("=") + off = value.lower() in ("false", "0", "f", "no", "off") + if name in ("-f", "--force"): + return None if off else "--force" + if name in ("-r", "--reset-hooks-path"): + return None if off else "--reset-hooks-path" + if name.startswith("-") and not name.startswith("--"): + # In a bundle (`-fr=false`) the value belongs to the last switch only; + # every switch before it is set. + letters = name[1:-1] if off else name[1:] + if "f" in letters: + return "--force" + if "r" in letters: + return "--reset-hooks-path" + return None + + +def _is_framework_path(value): + actual = os.path.normcase(os.path.realpath(os.path.expanduser(value))) + return any( + actual == os.path.normcase(os.path.realpath(os.path.expanduser(name))) + for name in FRAMEWORK_HOOKS_PATHS + ) + + +def guard(ctx): + flag = None + for argument in _install_arguments(ctx.argv): + flag = _destructive_flag(argument) + if flag: + break + if flag is None: + return None + if ctx.override("MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH"): + return None + hooks_path = ctx.run(["git", "config", "--global", "--get", "core.hooksPath"]) + if not hooks_path or not _is_framework_path(hooks_path): + return None + effect = "unset" if flag == "--reset-hooks-path" else "replace" + return ( + f"agent-guard[lefthook-hooks-path]: `lefthook install {flag}` would {effect} the " + f"framework's global core.hooksPath ({hooks_path}). Run plain `lefthook install`: " + "the ~/.claude/bin/lefthook shim keeps the hook in this repository's own hooks " + "dir. Override: MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1." + ) diff --git a/plugins/magpie-setup/skills/isolated-setup-install/step-p-sandbox-allowlists.md b/plugins/magpie-setup/skills/isolated-setup-install/step-p-sandbox-allowlists.md index 532f23420..834ba6eb5 100644 --- a/plugins/magpie-setup/skills/isolated-setup-install/step-p-sandbox-allowlists.md +++ b/plugins/magpie-setup/skills/isolated-setup-install/step-p-sandbox-allowlists.md @@ -103,10 +103,10 @@ acknowledge it explicitly; no silent proceed: > runs the framework's own logic (the `post-checkout` > sandbox-allowlist sync) **and then chains through to that > repo's own `.git/hooks/`**. Your per-repo hooks keep -> firing, and a repo with no hook is a clean no-op. A bundled -> `prek` PATH shim makes `prek install` write its shim into the -> repo-local `.git/hooks/` (via `--git-dir`) so the dispatcher -> can find it. +> firing, and a repo with no hook is a clean no-op. Bundled +> PATH shims for `prek` and `lefthook` make their `install` write +> into the repo-local `.git/hooks/` (prek via `--git-dir`, lefthook +> with the global config hidden) so the dispatcher can find it. > > Either way, whole-user scope is reversible: > `git config --global --unset core.hooksPath` restores per-repo @@ -326,34 +326,39 @@ for the full rationale + the validated behaviour matrix. with the original argv + inherited stdin, so a failing local `pre-commit` / `pre-push` still aborts the git operation. -2. **Install the hook-manager PATH shim** so the manager's install - writes its hook into the repo-local `.git/hooks/` (where the - dispatcher chains) instead of the shared dir. Copy +2. **Install the hook-manager PATH shim** so a manager's `install` writes its + hook into the repo-local `.git/hooks/` (where the dispatcher + chains) instead of the shared dir. Copy [`tools/agent-isolation/hook-manager-shim.sh`](../../../../tools/agent-isolation/hook-manager-shim.sh) into `~/.claude/bin/` (or symlink from `~/.claude-config/bin/`), mode `0755`, once for each adapter in the [hook-manager registry](../../../../tools/hook-manager/registry.json) with an install strategy other than `none` - that a repository the operator works in uses — `~/.claude/bin/prek` and - `~/.claude/bin/overcommit` today. A manager whose install strategy is - `none` needs no install fix and gets no shim. + that a repository the operator works in uses — `~/.claude/bin/prek`, + `~/.claude/bin/overcommit` and `~/.claude/bin/lefthook` today. + A manager whose install strategy is `none` needs no install fix + and gets no shim. A repository uses a manager when one of its config files is at the root (or tracked below it, for prek's nested projects) or one of its installed hooks runs it ([Detection](../../../../tools/hook-manager/README.md#detection)); for prek that is `prek.toml`, `.pre-commit-config.yaml` or - `.pre-commit-config.yml`, and for overcommit it is `.overcommit.yml`. - Install no shim for a manager no repository uses. The shim - takes the tool from the name it is installed under. For prek it - rewrites only `prek install` (injecting - `--git-dir "$(git rev-parse --git-common-dir)"` unless the caller - already passed `--git-dir`, asked for `--help`, or is outside a - git work tree); for overcommit it rewrites only - `overcommit --install` (or `-i`, `-fi`, `--inst`, anywhere in argv), - pointing `core.hooksPath` at - `/hooks`. **Every other invocation passes - through unchanged.** It is a no-op on hosts with no global - `core.hooksPath`. A host that installed the older `prek-shim.sh` - as `~/.claude/bin/prek` keeps working; replacing it is optional. + `.pre-commit-config.yml`; for overcommit, `.overcommit.yml`; for lefthook, + its root `lefthook.*`, `.lefthook.*` and `-local` names. + Install no shim for a manager no repository uses. + The shim takes the tool from the name it is installed under and rewrites only + the install action: for prek it injects + `--git-dir "$(git rev-parse --git-common-dir)"` unless the caller already + passed `--git-dir`; + for overcommit it rewrites `overcommit --install` (or `-i`, `-fi`, `--inst`, + anywhere in argv), pointing `core.hooksPath` at `/hooks`; + for lefthook it runs the install with `GIT_CONFIG_GLOBAL=/dev/null` while the + global config holds one, which hides the global `core.hooksPath` from + lefthook's own check and lets its target resolve to the common dir. + Each leaves the install alone when the caller asked for `--help` or is outside + a git work tree, and **every other invocation passes through unchanged.** + It is a no-op on hosts with no global `core.hooksPath`. + A host that installed the older `prek-shim.sh` as `~/.claude/bin/prek` keeps + working; replacing it is optional. 3. **Surface the PATH line for the operator to add** to their `~/.bashrc` / `~/.zshrc` (per the golden rules — never edit the diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py index 0b316d11f..0395e157d 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py @@ -21,4 +21,4 @@ for installs that do not carry the framework source (see `isolated.py`). """ -FRAMEWORK_FINGERPRINT = "sha256:db43bec52c1d2c7c" +FRAMEWORK_FINGERPRINT = "sha256:68ceca4bde12d81e" diff --git a/tools/agent-guard/README.md b/tools/agent-guard/README.md index 3c8e317b4..6ba1cdec5 100644 --- a/tools/agent-guard/README.md +++ b/tools/agent-guard/README.md @@ -136,6 +136,7 @@ way — see [Contributing guards](#contributing-guards)): | `mention` | `pr-management-triage` | `gh pr comment` / `gh issue comment` that `@`-mentions anyone other than the PR/issue author; **any** `@`-mention in `gh pr edit --body[-file]` | denoise: author-directed feedback never pings maintainers; body edits stay silent. Exempt: the operator commenting on their **own** PR/issue (author == authenticated `gh` user), and the `MAGPIE_ALLOW_MENTIONS=1` override | | `mark-ready` | `pr-management-triage` | adding `ready for maintainer review` while the PR head SHA has GitHub Actions runs awaiting approval | Golden rule 1b | | `security-language` | `security-issue-fix` | a CVE id / security-fix language in a **public** `gh pr create`/`gh pr edit` title/body (not comments) | public-PR scrubbing | +| `lefthook-hooks-path` | `setup-isolated-setup-install` | `lefthook install --force` / `-f` or `--reset-hooks-path` / `-r` while the global `core.hooksPath` is the framework's | the first writes over the dispatcher for every repository, the second unsets the whole-user setup; the shim hides the global path, so a plain `lefthook install` is all that is needed. Override: `MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1` | A denied command is **not** posted/run; the model is shown the reason and the deterministic fix (e.g. "use a backtick `` `login` `` instead of `@login`"). @@ -223,6 +224,7 @@ MAGPIE_ALLOW_MENTIONS=1 gh pr comment 123 --body "@reviewer please take anot MAGPIE_ALLOW_COAUTHOR=1 git commit -m "…" # not for AI co-authorship MAGPIE_ALLOW_MARK_READY=1 gh pr edit 123 --add-label "ready for maintainer review" MAGPIE_ALLOW_SECURITY_LANG=1 gh pr create --title "…" # disclosure already public +MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1 lefthook install --reset-hooks-path # whole-user setup is being removed on purpose MAGPIE_ALLOW_EMPTY_PUSH=1 git push --force … MAGPIE_ALLOW_VETTED_PATH=1 uv run --project tools/vetted-ops vetted-op-read … # developing the dispatcher ``` diff --git a/tools/agent-guard/src/agent_guard/__init__.py b/tools/agent-guard/src/agent_guard/__init__.py index cc847e17f..7d9077663 100644 --- a/tools/agent-guard/src/agent_guard/__init__.py +++ b/tools/agent-guard/src/agent_guard/__init__.py @@ -2337,8 +2337,9 @@ def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str # Only commands in these families are inspected; everything else takes the # instant fast path. Bundled and contributed guards alike operate on the -# `gh` / `git` outbound/destructive surface. -GUARDED_HEADS = frozenset({"gh", "git"}) +# `gh` / `git` outbound/destructive surface and on `lefthook`, whose +# `install` can undo the whole-user hook setup. +GUARDED_HEADS = frozenset({"gh", "git", "lefthook"}) # Colon-separated extra guard directories, searched before the discovered ones. # Skill-owned guards no longer need it (``framework_root`` finds them in place); @@ -2597,12 +2598,32 @@ def run(self, args: list[str]) -> str | None: return _run(args, cwd=self.cwd) +def guarded_head(argv: list[str]) -> str | None: + """The guarded command family a segment runs, or None. + + The head is usually the bare name, but a manager installed from a package + tree is reached by path (``node_modules/.bin/lefthook``), so the basename + counts too. + """ + if not argv: + return None + head = argv[0] + if head in GUARDED_HEADS: + return head + base = head.replace("\\", "/").rsplit("/", 1)[-1] + if base.lower().endswith(".exe"): + base = base[:-4] + return base if base in GUARDED_HEADS else None + + def command_kinds(seg: Segment) -> set[str]: """The command-family tags a segment matches, e.g. ``{"git", "git:commit"}``.""" kinds: set[str] = set() if not seg.argv: return kinds - head = seg.argv[0] + head = guarded_head(seg.argv) + if head is None: + return kinds kinds.add(head) if head == "git": # A TRIGGERS = ["git:commit"] contributed guard must still fire behind @@ -2615,6 +2636,11 @@ def command_kinds(seg: Segment) -> set[str]: # Same for TRIGGERS = ["gh:pr"] behind `gh -R pr ...`. sub = gh_subcommand(seg.argv) kinds.add(f"{head}:{sub[0] if sub else seg.argv[1]}") + elif head == "lefthook": + # lefthook accepts global flags before the subcommand. + lefthook_sub = next((tok for tok in seg.argv[1:] if not tok.startswith("-")), None) + if lefthook_sub: + kinds.add(f"lefthook:{lefthook_sub}") return kinds @@ -2782,7 +2808,7 @@ def dispatch(command: str, cwd: str | None = None, harness: str | None = None) - if reason: return reason continue - if not seg.argv or seg.argv[0] not in GUARDED_HEADS: + if not seg.argv or guarded_head(seg.argv) is None: continue # fast path — non-guarded command family # Bundled guards (trusted, in-process; each self-filters its command). for builtin in BUILTIN_GUARDS: diff --git a/tools/agent-guard/tests/test_guards.py b/tools/agent-guard/tests/test_guards.py index d7d3454c4..b829f40ed 100644 --- a/tools/agent-guard/tests/test_guards.py +++ b/tools/agent-guard/tests/test_guards.py @@ -51,6 +51,27 @@ def _stub(args, cwd=None): return _stub +# --------------------------------------------------------------------------- # +# guarded_head (shared helper, stays in the engine) +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize( + "head, expected", + [ + ("lefthook", "lefthook"), + ("./node_modules/.bin/lefthook", "lefthook"), + (r"node_modules\.bin\lefthook", "lefthook"), + (r"C:\tools\lefthook.exe", "lefthook"), + ("lefthook.exe", "lefthook"), + ("/usr/bin/gh", "gh"), + ("curl", None), + ], +) +def test_guarded_head_recognizes_paths_from_every_platform(head, expected): + assert agent_guard.guarded_head([head, "install"]) == expected + + # --------------------------------------------------------------------------- # # find_mentions / strip_code (shared helper, stays in the engine) # --------------------------------------------------------------------------- # diff --git a/tools/agent-guard/tests/test_skill_guards.py b/tools/agent-guard/tests/test_skill_guards.py index 99cdb3193..e2bb7869a 100644 --- a/tools/agent-guard/tests/test_skill_guards.py +++ b/tools/agent-guard/tests/test_skill_guards.py @@ -38,6 +38,11 @@ if (REPO_ROOT / "plugins" / "magpie-security" / "skills" / "issue-fix" / "guards").is_dir() else REPO_ROOT / "skills" / "security-issue-fix" / "guards" ) +SETUP_GUARDS = ( + REPO_ROOT / "plugins" / "magpie-setup" / "skills" / "isolated-setup-install" / "guards" + if (REPO_ROOT / "plugins" / "magpie-setup" / "skills" / "isolated-setup-install" / "guards").is_dir() + else REPO_ROOT / "skills" / "setup-isolated-setup-install" / "guards" +) @pytest.fixture(autouse=True) @@ -47,9 +52,13 @@ def _wire_skill_guards(monkeypatch): "MAGPIE_ALLOW_MENTIONS", "MAGPIE_ALLOW_MARK_READY", "MAGPIE_ALLOW_SECURITY_LANG", + "MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH", ): monkeypatch.delenv(name, raising=False) - monkeypatch.setenv("MAGPIE_GUARD_DIRS", f"{TRIAGE_GUARDS}{os.pathsep}{SECURITY_GUARDS}") + monkeypatch.setenv( + "MAGPIE_GUARD_DIRS", + f"{TRIAGE_GUARDS}{os.pathsep}{SECURITY_GUARDS}{os.pathsep}{SETUP_GUARDS}", + ) def fake_run(handler): @@ -80,6 +89,7 @@ def test_guard_files_exist(): assert (TRIAGE_GUARDS / "mention.py").is_file() assert (TRIAGE_GUARDS / "mark_ready.py").is_file() assert (SECURITY_GUARDS / "security_language.py").is_file() + assert (SETUP_GUARDS / "lefthook_hooks_path.py").is_file() # --- mention guard (skill-owned) ------------------------------------------- # @@ -566,3 +576,90 @@ def test_prefix_wrappers_reach_the_mention_guard(monkeypatch, prefix): def test_an_override_on_env_still_counts(monkeypatch): monkeypatch.setattr(agent_guard, "_run", fake_run(gh_stub(author="alice"))) assert dispatch('env MAGPIE_ALLOW_MENTIONS=1 gh pr comment 5 --body "@bob ping"') is None + + +# --- lefthook hooks-path guard (skill-owned) ------------------------------- # + +FRAMEWORK_HOOKS = "~/.claude/git-hooks" + + +def hooks_path_stub(value): + def handler(args): + if args[:2] == ["git", "config"]: + return value + return None + + return handler + + +@pytest.mark.parametrize("hooks_path", [FRAMEWORK_HOOKS, "~/.claude-config/git-hooks"]) +@pytest.mark.parametrize( + "command, flag", + [ + ("lefthook install --force", "--force"), + ("lefthook install -f", "--force"), + ("lefthook install --force=true", "--force"), + ("lefthook install --force=1", "--force"), + ("lefthook install --reset-hooks-path", "--reset-hooks-path"), + ("lefthook install --reset-hooks-path=true", "--reset-hooks-path"), + ("lefthook install -r", "--reset-hooks-path"), + ("lefthook install -fr", "--force"), + ("lefthook install -fr=false", "--force"), + ("lefthook install -rf=false", "--reset-hooks-path"), + ("lefthook --verbose install --force", "--force"), + ("./node_modules/.bin/lefthook install --force", "--force"), + ], +) +def test_lefthook_destructive_install_denied(monkeypatch, hooks_path, command, flag): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(hooks_path))) + reason = dispatch(command) + assert reason and "lefthook-hooks-path" in reason and flag in reason + + +@pytest.mark.parametrize( + "command", + [ + "lefthook install", + "lefthook install --verbose", + "lefthook run pre-commit", + "lefthook --version", + ], +) +def test_lefthook_plain_invocations_allowed(monkeypatch, command): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(FRAMEWORK_HOOKS))) + assert dispatch(command) is None + + +@pytest.mark.parametrize( + "command", + [ + "lefthook install --force=false", + "lefthook install --reset-hooks-path=false", + "lefthook install -f=false", + ], +) +def test_lefthook_switched_off_destructive_flag_allowed(monkeypatch, command): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(FRAMEWORK_HOOKS))) + assert dispatch(command) is None + + +def test_lefthook_destructive_install_allowed_for_another_global_path(monkeypatch): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub("/opt/other-hooks"))) + assert dispatch("lefthook install --force") is None + + +def test_lefthook_destructive_install_allowed_without_a_global_path(monkeypatch): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(None))) + assert dispatch("lefthook install --reset-hooks-path") is None + + +def test_lefthook_destructive_install_override(monkeypatch): + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(FRAMEWORK_HOOKS))) + assert dispatch("MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1 lefthook install --force") is None + + +def test_lefthook_framework_path_comparison_ignores_case(monkeypatch): + monkeypatch.setattr(os.path, "normcase", str.lower) + monkeypatch.setattr(agent_guard, "_run", fake_run(hooks_path_stub(FRAMEWORK_HOOKS.upper()))) + reason = dispatch("lefthook install --force") + assert reason and "lefthook-hooks-path" in reason diff --git a/tools/agent-isolation/README.md b/tools/agent-isolation/README.md index 4ef4f5b34..c7c923fe9 100644 --- a/tools/agent-isolation/README.md +++ b/tools/agent-isolation/README.md @@ -97,8 +97,8 @@ per runtime — see [`docs/adapters/add-a-harness.md`](../../docs/adapters/add-a | [`claude-term-bg.sh`](claude-term-bg.sh) | **Opt-in quality-of-life helper (not a security control).** Keeps a calm baseline background and tints it only when Claude genuinely wants you to act (never while working, and never when it merely *finished* a turn), so a window you've tabbed away from can't sit blocked unnoticed. Distinguishes "blocked on a decision" from "finished and idle" — which look identical at the `Stop` event — via three signals across six hooks: `Stop` → `stop` (heuristic — tints only if the final assistant message reads as a question/request; a completion stays calm; needs `python3`/`python`, else defaults calm); `PreToolUse` (matcher `AskUserQuestion`) → `wait` (exact — a structured question was posed); `PostToolUse` (matcher `*`) → `reset` (calm while working, and clears the tint the instant you approve a permission prompt or answer a question); `Notification` → `notify` (tints for permission prompts only — the plain idle ping is a no-op so it can't wipe a pending question's tint); and `UserPromptSubmit` + `SessionStart` → `reset` (you replied / fresh session clears any stale tint). Writes the OSC escape to the Claude pty discovered by walking the process tree (hooks have no controlling tty); the only deterministic reset is an explicit `CLAUDE_RESET_BG` colour via OSC 11 (iTerm2 ignores OSC 111). Colours overridable via `CLAUDE_WAIT_BG` / `CLAUDE_RESET_BG`. Tested on iTerm2 + macOS; fail-soft elsewhere. See [`docs/setup/secure-agent-setup.md` → *Waiting-for-input terminal tint*](../../docs/setup/secure-agent-setup.md#waiting-for-input-terminal-tint). | | [`sandbox-add-project-root.sh`](sandbox-add-project-root.sh) | Adds the current adopter repo's project root (and, with `--all-worktrees`, every linked git worktree's working dir) as an explicit absolute path to `sandbox.filesystem.allowRead` and `allowWrite` in the project-local, gitignored `/.claude/settings.local.json` — one entry per worktree, each in that worktree's own settings file. Defensive against [issue #197](https://github.com/apache/magpie/issues/197) — `allowRead: ["."]` does not in practice cover CWD because the harness pre-resolves the `.` literal away from the read side. The same harness behaviour drops the committed home-directory entries, so it also adds the dev-tool paths `prek` and `uv` need (`~/.gitconfig`, `~/.config/git`, `~/.cache`, `~/.local/share/uv`, `~/.local/bin`; the cache and uv dirs writable) as absolute paths — never the credential paths; `--no-tool-paths` skips them. See [`prek` or `uv` not found inside the sandbox](../../docs/setup/sandbox-troubleshooting.md#prek-or-uv-not-found-or-cannot-write-its-cache-inside-the-sandbox). Never modifies user-scope or committed project-scope. Idempotent, atomic, tolerant of missing prereqs. Invoked from `setup-isolated-setup-install`, `/magpie-setup:setup` (adopt / upgrade / worktree-init), and the `post-checkout` git hook installed by `/magpie-setup:setup adopt`. | | [`git-global-post-checkout.sh`](git-global-post-checkout.sh) | Universal `post-checkout` git hook installed at `~/.claude/git-hooks/post-checkout` when the operator picks the **simple whole-user** flavour in `setup-isolated-setup-install`. Activated by `git config --global core.hooksPath ~/.claude/git-hooks/` so every `git checkout` / `git clone` / `git worktree add` across the host invokes it. Best-effort + idempotent + `\|\| true`: invokes `sandbox-add-project-root.sh` for any worktree with a `.claude/` directory. Trade-off documented in [`docs/setup/secure-agent-setup.md` → *Per-project vs whole-user scope*](../../docs/setup/secure-agent-setup.md#per-project-vs-whole-user-scope): `core.hooksPath` shadows per-repo `.git/hooks/*` across every repo on the host. The **dispatcher** flavour (below) supersedes this file. | -| [`git-hook-dispatcher.sh`](git-hook-dispatcher.sh) | Universal, basename-keyed git-hook **dispatcher** for the **whole-user dispatcher** flavour. Symlinked to every hook name under `~/.claude/git-hooks/`. For each git operation it runs the framework's own logic for that hook type (the `post-checkout` sandbox-allowlist sync) **and then chains through to the repo-local `.git/hooks/`** (resolved via `git rev-parse --git-common-dir`, worktree-safe; `exec`-ed with original argv + inherited stdin so a failing local hook still aborts the git op). Restores per-repo hooks (prek / overcommit / pre-commit / husky / hand-written) under global `core.hooksPath`; a repo with no local hook is a clean no-op. Outside the sandbox it also gives every hook manager in the [hook-manager registry](../hook-manager/registry.json) its host-only home (the contract's home rule), and before chaining into an overcommit hook it exports the same `core.hooksPath` override the shim used so overcommit's run-time self-check accepts the hook. See [`docs/setup/secure-agent-setup.md` → *Whole-user with the per-repo dispatcher*](../../docs/setup/secure-agent-setup.md#whole-user-with-the-per-repo-dispatcher). | -| [`hook-manager-shim.sh`](hook-manager-shim.sh) | Transparent front for a git-hook manager, installed as `~/.claude/bin/` (with `~/.claude/bin` prepended to PATH) for the whole-user dispatcher flavour, one install per adapter in the [hook-manager registry](../hook-manager/registry.json) whose install strategy is not `none` (`prek` and `overcommit` today). The tool is the name it is installed under. Rewrites **only** the adapter's install action so the hook lands in the repo-local `.git/hooks/` where the dispatcher chains, instead of the shared `core.hooksPath` dir — for prek, injecting `--git-dir "$(git rev-parse --git-common-dir)"` unless the caller already passed `--git-dir`, asked for `--help`, or is outside a git work tree; for overcommit, finding `--install` or its `-i` alias anywhere in argv and setting `core.hooksPath` to `/hooks` in the next free `GIT_CONFIG_*` slot. Every other invocation passes through unchanged; no-op on hosts with no global `core.hooksPath`. Implements the install-target rule of the [hook-manager contract](../hook-manager/README.md). | +| [`git-hook-dispatcher.sh`](git-hook-dispatcher.sh) | Universal, basename-keyed git-hook **dispatcher** for the **whole-user dispatcher** flavour. Symlinked to every hook name under `~/.claude/git-hooks/`. For each git operation it runs the framework's own logic for that hook type (the `post-checkout` sandbox-allowlist sync) **and then chains through to the repo-local `.git/hooks/`** (resolved via `git rev-parse --git-common-dir`, worktree-safe; `exec`-ed with original argv + inherited stdin so a failing local hook still aborts the git op). Restores per-repo hooks (prek / overcommit / pre-commit / lefthook / husky / hand-written) under global `core.hooksPath`; a repo with no local hook is a clean no-op. Outside the sandbox it also gives every hook manager in the [hook-manager registry](../hook-manager/registry.json) its host-only home (the contract's home rule), and before chaining into an overcommit hook it exports the same `core.hooksPath` override the shim used so overcommit's run-time self-check accepts the hook. See [`docs/setup/secure-agent-setup.md` → *Whole-user with the per-repo dispatcher*](../../docs/setup/secure-agent-setup.md#whole-user-with-the-per-repo-dispatcher). | +| [`hook-manager-shim.sh`](hook-manager-shim.sh) | Transparent front for a git-hook manager, installed as `~/.claude/bin/` (with `~/.claude/bin` prepended to PATH) for the whole-user dispatcher flavour, one install per adapter in the [hook-manager registry](../hook-manager/registry.json) whose install strategy is not `none` (`prek`, `overcommit` and `lefthook` today). The tool is the name it is installed under. Rewrites **only** the adapter's install action so the hook lands in the repo-local `.git/hooks/` where the dispatcher chains, instead of the shared `core.hooksPath` dir — for prek, injecting `--git-dir "$(git rev-parse --git-common-dir)"` unless the caller already passed `--git-dir`, asked for `--help`, or is outside a git work tree; for overcommit, finding `--install` or its `-i` alias anywhere in argv and setting `core.hooksPath` to `/hooks` in the next free `GIT_CONFIG_*` slot; for lefthook, running the install with `GIT_CONFIG_GLOBAL=/dev/null` while the global config holds a `core.hooksPath`, so lefthook's own `core.hooksPath` refusal does not fire and a host without one keeps its global settings. Every other invocation passes through unchanged; no-op on hosts with no global `core.hooksPath`. Implements the install-target rule of the [hook-manager contract](../hook-manager/README.md). | ## Usage at a glance diff --git a/tools/agent-isolation/hook-manager-shim.sh b/tools/agent-isolation/hook-manager-shim.sh index f8253d854..b899aca0b 100755 --- a/tools/agent-isolation/hook-manager-shim.sh +++ b/tools/agent-isolation/hook-manager-shim.sh @@ -17,10 +17,11 @@ # under the License. # # hook-manager-shim.sh — a transparent front for a git-hook manager -# (prek and overcommit today; see tools/hook-manager/registry.json), installed as -# `~/.claude/bin/` with `~/.claude/bin` prepended to PATH so it -# shadows the real binary. The tool is the name it is installed -# under (`basename "$0"`), so one script serves every adapter. +# (prek, overcommit and lefthook today; see tools/hook-manager/registry.json), +# installed as `~/.claude/bin/` with `~/.claude/bin` prepended +# to PATH so it shadows the real binary. The tool is the name it is +# installed under (`basename "$0"`), so one script serves every +# adapter. # # WHY THIS EXISTS # --------------- @@ -53,6 +54,15 @@ # git-dir-flag: inject ` ` (prek: --git-dir) # hooks-path-env: append GIT_CONFIG_* so the manager writes to # `/hooks` (overcommit: --install, -i) +# hide-global-config: run the install with +# GIT_CONFIG_GLOBAL=/dev/null, for a manager that writes to +# the common dir itself but refuses while a global +# core.hooksPath is set (lefthook). The override applies only +# when the global config actually holds a core.hooksPath, so an +# ordinary host keeps its credential and url.* settings, and +# even then it applies with `--force` / `--reset-hooks-path`: +# through this shim those must not write into the shared dir or +# unset the whole-user setup. Run the real binary to bypass it. # * every other invocation (`prek run`, `prek autoupdate`, # `--version`, bare `prek`, ...) is passed through to the real # binary **unchanged**. @@ -70,6 +80,7 @@ tool="$(basename "$0")" # BEGIN generated by tools/dev/sync-hook-managers.py; do not edit adapter() { case "$1" in + lefthook) install_subcommand=install; install_strategy=hide-global-config ;; overcommit) install_subcommand=--install; install_strategy=hooks-path-env; install_aliases='--install -i' ;; prek) install_subcommand=install; install_strategy=git-dir-flag; install_flag=--git-dir ;; *) return 1 ;; @@ -153,32 +164,49 @@ if [ "$install_invoked" = 0 ]; then exec "$real_tool" "$@" fi -# Respect a target the caller chose, and never interfere with help output. -inject=1 -for _a in "$@"; do - case "$_a" in - -h|--help) inject=0; break ;; - esac - if [ -n "${install_flag:-}" ]; then - case "$_a" in - "$install_flag"|"$install_flag"=*) inject=0; break ;; - esac - fi -done +# --- install strategies: one hand-written branch per install_strategy ------ +# (tools/dev/sync-hook-managers.py lists them in STRATEGIES and generates only +# the adapter() block above.) +case "$install_strategy" in + git-dir-flag) + # Only positional actions use this strategy; the action is argv[0]. + shift # drop the install subcommand + # Respect a target the caller chose, and never interfere with help output. + inject=1 + for _a in "$@"; do + case "$_a" in + -h|--help) inject=0; break ;; + "$install_flag"|"$install_flag"=*) inject=0; break ;; + esac + done -if [ "$inject" = 1 ]; then - if gitdir="$(git rev-parse --git-common-dir 2>/dev/null)"; then - case "$gitdir" in - /*) : ;; # already absolute - *) gitdir="$(cd "$gitdir" 2>/dev/null && pwd -P || echo "$gitdir")" ;; - esac - case "$install_strategy" in - git-dir-flag) - # Only positional actions use this strategy; the action is argv[0]. - shift + if [ "$inject" = 1 ]; then + if gitdir="$(git rev-parse --git-common-dir 2>/dev/null)"; then + case "$gitdir" in + /*) : ;; # already absolute + *) gitdir="$(cd "$gitdir" 2>/dev/null && pwd -P || echo "$gitdir")" ;; + esac exec "$real_tool" "$install_subcommand" "$install_flag" "$gitdir" "$@" - ;; - hooks-path-env) + fi + # not in a git work tree: let the real tool produce its own error + fi + exec "$real_tool" "$install_subcommand" "$@" + ;; + hooks-path-env) + # Never interfere with help output. + inject=1 + for _a in "$@"; do + case "$_a" in + -h|--help) inject=0; break ;; + esac + done + + if [ "$inject" = 1 ]; then + if gitdir="$(git rev-parse --git-common-dir 2>/dev/null)"; then + case "$gitdir" in + /*) : ;; # already absolute + *) gitdir="$(cd "$gitdir" 2>/dev/null && pwd -P || echo "$gitdir")" ;; + esac # Append at the next index so a caller's GIT_CONFIG_* entries survive. _i="${GIT_CONFIG_COUNT:-0}" case "$_i" in @@ -188,10 +216,31 @@ if [ "$inject" = 1 ]; then export "GIT_CONFIG_VALUE_$_i=${gitdir%/}/hooks" export GIT_CONFIG_COUNT=$((_i + 1)) exec "$real_tool" "$@" - ;; - esac - fi - # not in a git work tree: let the real tool produce its own error -fi + fi + # not in a git work tree: let the real tool produce its own error + fi + ;; + hide-global-config) + shift # drop the install subcommand + # Never interfere with help output. + for _a in "$@"; do + case "$_a" in + -h|--help) exec "$real_tool" "$install_subcommand" "$@" ;; + esac + done + + if git rev-parse --git-common-dir >/dev/null 2>&1; then + # Hide the global config only when it holds a core.hooksPath to hide: the + # override also costs the install the operator's credential.helper, + # url.*.insteadOf and safe.directory, and the manager can clone remote + # configs while installing. + if [ -n "$(git config --global --get core.hooksPath 2>/dev/null)" ]; then + GIT_CONFIG_GLOBAL=/dev/null exec "$real_tool" "$install_subcommand" "$@" + fi + fi + # not in a git work tree: let the real tool produce its own error + exec "$real_tool" "$install_subcommand" "$@" + ;; +esac exec "$real_tool" "$@" diff --git a/tools/agent-isolation/tests/test_hook_manager_shim.py b/tools/agent-isolation/tests/test_hook_manager_shim.py index 4380191a7..18d19fdef 100644 --- a/tools/agent-isolation/tests/test_hook_manager_shim.py +++ b/tools/agent-isolation/tests/test_hook_manager_shim.py @@ -53,6 +53,54 @@ def _setup(tmp_path: Path, name: str = "prek") -> tuple[dict[str, str], Path]: return env, argv +def _setup_lefthook(tmp_path: Path) -> tuple[dict[str, str], Path, Path]: + """A fake lefthook that refuses while a global core.hooksPath is visible. + + It records the global config it was given, then installs a hook where + `git rev-parse --git-path hooks` resolves, as lefthook does. + """ + real = tmp_path / "real" + real.mkdir() + record = tmp_path / "record" + shared = tmp_path / "shared-hooks" + (real / "lefthook").write_text( + "#!/bin/sh\n" + f'printf "GIT_CONFIG_GLOBAL=%s\\n" "${{GIT_CONFIG_GLOBAL-}}" > {record}\n' + f'printf "%s\\n" "$@" >> {record}\n' + 'for arg in "$@"; do\n' + ' case "$arg" in -h|--help) exit 0 ;; esac\n' + "done\n" + 'if [ "$1" = install ]; then\n' + ' if [ "${GIT_CONFIG_GLOBAL-}" != /dev/null ] ' + '&& [ -n "$(git config --global --get core.hooksPath 2>/dev/null)" ]; then\n' + ' echo "core.hooksPath is set globally" >&2\n' + " exit 1\n" + " fi\n" + ' hooks="$(git rev-parse --path-format=absolute --git-path hooks)"\n' + ' mkdir -p "$hooks" && printf "#!/bin/sh\\n" > "$hooks/pre-commit"\n' + "fi\n" + ) + (real / "lefthook").chmod(0o755) + shims = tmp_path / "shims" + shims.mkdir() + (shims / "lefthook").symlink_to(SHIM) + home = tmp_path / "home" + home.mkdir() + (home / ".gitconfig").write_text(f"[core]\n\thooksPath = {shared}\n") + env = dict(os.environ) + for name in ("GIT_CONFIG_GLOBAL", "GIT_CONFIG_COUNT", "GIT_CONFIG_KEY_0", "GIT_CONFIG_VALUE_0"): + env.pop(name, None) + env.update(PATH=f"{shims}:{real}:/usr/bin:/bin", HOME=str(home), GIT_CONFIG_NOSYSTEM="1") + return env, record, shared + + +def _set_global_hooks_path(env: dict[str, str], value: str | None) -> None: + """Rewrite the fake home's global config: a `core.hooksPath`, or none.""" + (Path(env["HOME"]) / ".gitconfig").write_text( + f"[core]\n\thooksPath = {value}\n" if value else "[user]\n\tname = t\n" + ) + + def _run( env: dict[str, str], cwd: Path, *args: str, name: str = "prek" ) -> subprocess.CompletedProcess[str]: @@ -212,6 +260,117 @@ def test_a_linked_worktree_targets_the_main_checkout(tmp_path: Path) -> None: ] +def test_lefthook_install_hides_the_global_hooks_path(tmp_path: Path) -> None: + env, record, shared = _setup_lefthook(tmp_path) + repo = _repo(tmp_path) + assert _run(env, repo, "install", name="lefthook").returncode == 0 + assert record.read_text().splitlines() == ["GIT_CONFIG_GLOBAL=/dev/null", "install"] + assert (repo / ".git" / "hooks" / "pre-commit").is_file() + assert not (shared / "pre-commit").exists() + + +def test_lefthook_install_hides_a_custom_global_hooks_path(tmp_path: Path) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + other = tmp_path / "other-hooks" + _set_global_hooks_path(env, str(other)) + repo = _repo(tmp_path) + assert _run(env, repo, "install", name="lefthook").returncode == 0 + assert record.read_text().splitlines() == ["GIT_CONFIG_GLOBAL=/dev/null", "install"] + assert (repo / ".git" / "hooks" / "pre-commit").is_file() + assert not (other / "pre-commit").exists() + + +def test_lefthook_install_without_a_global_hooks_path_keeps_the_global_config( + tmp_path: Path, +) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + _set_global_hooks_path(env, None) + repo = _repo(tmp_path) + assert _run(env, repo, "install", name="lefthook").returncode == 0 + assert record.read_text().splitlines() == ["GIT_CONFIG_GLOBAL=", "install"] + assert (repo / ".git" / "hooks" / "pre-commit").is_file() + + +def test_lefthook_install_in_a_linked_worktree_targets_the_main_checkout( + tmp_path: Path, +) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + repo = _repo(tmp_path) + subprocess.run( + ["git", "-C", str(repo), "-c", "commit.gpgsign=false", "commit", "-q", "--allow-empty", "-m", "x"], + check=True, + env=dict( + os.environ, + GIT_AUTHOR_NAME="t", + GIT_AUTHOR_EMAIL="t@t", + GIT_COMMITTER_NAME="t", + GIT_COMMITTER_EMAIL="t@t", + ), + ) + worktree = tmp_path / "wt" + subprocess.run( + ["git", "-C", str(repo), "worktree", "add", "-q", str(worktree)], check=True + ) + assert _run(env, worktree, "install", name="lefthook").returncode == 0 + assert record.read_text().splitlines()[0] == "GIT_CONFIG_GLOBAL=/dev/null" + assert (repo / ".git" / "hooks" / "pre-commit").is_file() + + +@pytest.mark.parametrize( + "args", + [ + ["install", "--force"], + ["install", "-f"], + ["install", "--reset-hooks-path"], + ["install", "-r"], + ["install", "-fr"], + ], +) +def test_lefthook_destructive_flags_cannot_reach_the_global_dir( + tmp_path: Path, args: list[str] +) -> None: + env, record, shared = _setup_lefthook(tmp_path) + repo = _repo(tmp_path) + assert _run(env, repo, *args, name="lefthook").returncode == 0 + assert record.read_text().splitlines()[0] == "GIT_CONFIG_GLOBAL=/dev/null" + assert (repo / ".git" / "hooks" / "pre-commit").is_file() + assert not (shared / "pre-commit").exists() + + +def test_lefthook_other_invocations_pass_through(tmp_path: Path) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + assert ( + _run(env, _repo(tmp_path), "run", "pre-commit", name="lefthook").returncode == 0 + ) + assert record.read_text().splitlines() == [ + "GIT_CONFIG_GLOBAL=", + "run", + "pre-commit", + ] + + +@pytest.mark.parametrize("help_flag", ["-h", "--help"]) +def test_lefthook_help_passes_through(tmp_path: Path, help_flag: str) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + assert ( + _run(env, _repo(tmp_path), "install", help_flag, name="lefthook").returncode == 0 + ) + assert record.read_text().splitlines() == [ + "GIT_CONFIG_GLOBAL=", + "install", + help_flag, + ] + + +def test_lefthook_install_outside_a_work_tree_passes_through(tmp_path: Path) -> None: + env, record, _shared = _setup_lefthook(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + env["GIT_CEILING_DIRECTORIES"] = str(tmp_path) + assert _run(env, outside, "install", name="lefthook").returncode == 1 + assert record.read_text().splitlines() == ["GIT_CONFIG_GLOBAL=", "install"] + + @pytest.mark.parametrize( "args", [ diff --git a/tools/dev/sync-hook-managers.py b/tools/dev/sync-hook-managers.py index 331f10b6f..d1764295d 100755 --- a/tools/dev/sync-hook-managers.py +++ b/tools/dev/sync-hook-managers.py @@ -55,7 +55,7 @@ SETTINGS = (Path(".claude/settings.json"), Path("tools/sandbox-lint/expected.json")) #: Install strategies the shim implements. A new one lands with its shim branch. -STRATEGIES = {"git-dir-flag", "hooks-path-env", "none"} +STRATEGIES = {"git-dir-flag", "hooks-path-env", "hide-global-config", "none"} _NAME = re.compile(r"^[a-z][a-z0-9-]*$") _SUBCOMMAND = re.compile(r"^(?:--)?[a-z][a-z0-9-]*$") @@ -136,6 +136,8 @@ def validate(registry: dict[str, Any]) -> list[str]: errors.append( f"{where}.install.flag: git-dir-flag needs the long option that takes the git dir" ) + elif strategy != "git-dir-flag" and "flag" in install: + errors.append(f"{where}.install.flag: strategy {strategy!r} takes no flag") else: for key in ("flag", "aliases"): if key in install: diff --git a/tools/dev/tests/test_sync_hook_managers.py b/tools/dev/tests/test_sync_hook_managers.py index ecca144db..e120c52b8 100644 --- a/tools/dev/tests/test_sync_hook_managers.py +++ b/tools/dev/tests/test_sync_hook_managers.py @@ -159,13 +159,22 @@ def test_strategy_none_adapter_omits_shim_and_needs_no_subcommand(registry: dict " prek) install_subcommand=install; install_strategy=git-dir-flag; install_flag=--git-dir ;;" in shim ) + # Lefthook needs a rewrite too, but has no flag to inject + assert " lefthook) install_subcommand=install; install_strategy=hide-global-config ;;" in shim def test_strategy_none_with_flag_is_refused(registry: dict[str, Any]) -> None: broken = copy.deepcopy(registry) broken["adapters"]["simple-git-hooks"]["install"]["flag"] = "--git-dir" errors = mod.validate(broken) - assert any("strategy 'none' takes no flag" in e for e in errors), errors + assert any("takes no flag" in e for e in errors), errors + + +def test_hide_global_config_with_flag_is_refused(registry: dict[str, Any]) -> None: + broken = copy.deepcopy(registry) + broken["adapters"]["lefthook"]["install"]["flag"] = "--git-dir" + errors = mod.validate(broken) + assert any("takes no flag" in e for e in errors), errors def test_strategy_none_with_aliases_is_refused(registry: dict[str, Any]) -> None: diff --git a/tools/hook-manager/README.md b/tools/hook-manager/README.md index d9cfbafec..fa04a4ed2 100644 --- a/tools/hook-manager/README.md +++ b/tools/hook-manager/README.md @@ -74,7 +74,7 @@ It leaves the install alone when the caller already chose a target, asked for he | Strategy | What the shim does | Status | |---|---|---| | `git-dir-flag` | Inserts ` ` after the install subcommand (prek: `--git-dir`; prek refuses a bare install while a global `core.hooksPath` is set). | implemented | -| `hide-global-config` | Runs the install with `GIT_CONFIG_GLOBAL=/dev/null`, for a manager that writes to the common dir but refuses while a global `core.hooksPath` is set (pre-commit, lefthook). | planned | +| `hide-global-config` | Runs the install with `GIT_CONFIG_GLOBAL=/dev/null` whenever the global config holds a `core.hooksPath`, for a manager that writes to the common dir but refuses while one is set (pre-commit, lefthook); a host without one keeps its global `credential.helper` / `url.*.insteadOf` settings. The override also applies to an install carrying `--force` or `--reset-hooks-path`, so neither can reach the shared directory through the shim; run the real binary to do that. | implemented | | `hooks-path-env` | Runs the install with `core.hooksPath=/hooks` in the next free `GIT_CONFIG_*` slot, preserving a caller's existing entries, for a manager that writes into whatever `core.hooksPath` names (overcommit, CaptainHook). The value must be that absolute path: git treats an empty `core.hooksPath` as set, and hooks then resolve against the filesystem root. The run-time override is exported, so git commands the chained hook starts inherit it, including ones aimed at another repository; only the manager's self-check needs it, so that leak is a known limitation. | implemented | | `none` | The manager already writes to the repository's own hooks directory whatever the global value (simple-git-hooks, cargo-husky, the JGit-based Maven plugins). | implemented | @@ -116,6 +116,12 @@ Detection decides what the setup configures *for a repository*; it never decides How prek configures itself, from its source (v0.5.7): it reads one project config per directory, `prek.toml`, then `.pre-commit-config.yaml`, then `.pre-commit-config.yml` (`crates/prek-consts/src/lib.rs`); searches upward from the current directory to the git root for the workspace root, and below it for nested project configs, honouring `.gitignore` and `.prekignore` (`crates/prek/src/workspace.rs`); takes `--config ` instead, which disables discovery and is baked into the installed hook; and reads a user-level `prek.toml` under the platform config directory that holds only update and display options, so it says nothing about whether a repository uses prek. The two YAML names are shared with pre-commit, so a repository with one uses both managers as far as detection goes; configuring a home the repository's hooks do not end up using is harmless. +How lefthook configures itself, from its source (v2.2.2): it reads `lefthook`, `.lefthook` or `.config/lefthook` with one of `.yml`, `.yaml`, `.json`, `.jsonc`, `.toml`, and the same three names with a `-local` suffix as the repository's local override (`internal/config/loader.go`). +Only the bare root names are in the registry: the contract's `detect.config_files` holds file names, not directories, so a `.config/lefthook.yml` at the repository root is not detected by rule 1; the installed hook's `call_lefthook` marker still catches it by rule 3. +`lefthook install` writes to `$(git rev-parse --git-path hooks)` and refuses while a scoped `git config --local` or `--global` read returns any `core.hooksPath` (`internal/command/install.go`). +`GIT_CONFIG_GLOBAL=/dev/null` hides the global value from that read, and the path then resolves to the common directory, linked worktrees included. +Remote configs are cloned into `/lefthook-remotes`, inside `.git`, so the registry omits `home`. + ## Registry [`registry.json`](registry.json) holds one adapter per manager under `adapters.`: @@ -153,7 +159,7 @@ Its operations are carried out by the scripts generated from the registry: | Operation | Run by | Invocation | Behaviour | |---|---|---|---| -| Install rewrite | [`hook-manager-shim.sh`](../agent-isolation/hook-manager-shim.sh), installed as `~/.claude/bin/` | ` [args…]`, e.g. `prek install` or `overcommit --install` (also `-i`, or after other options) | Applies the adapter's install strategy — for prek, runs `prek install --git-dir [args…]`; for overcommit, runs `overcommit --install` or `-i` with `core.hooksPath` pointed at `/hooks`, appending to any existing `GIT_CONFIG_*` entries. Left unchanged when `args` already carry the target flag or `-h` / `--help`, or outside a git work tree. Every other subcommand passes through to the real binary, the next `` on `PATH` that is not the shim. Exits 127 when the name it is installed under has no adapter or no real binary is found. | +| Install rewrite | [`hook-manager-shim.sh`](../agent-isolation/hook-manager-shim.sh), installed as `~/.claude/bin/` | ` [args…]`, e.g. `prek install`, `lefthook install` or `overcommit --install` (also `-i`, or after other options) | Applies the adapter's install strategy: `git-dir-flag` runs `prek install --git-dir [args…]`; `hooks-path-env` runs `overcommit --install` or `-i` with `core.hooksPath` pointed at `/hooks`, appending to any existing `GIT_CONFIG_*` entries; `hide-global-config` runs the install with `GIT_CONFIG_GLOBAL=/dev/null` when the global config holds a `core.hooksPath`. Left unchanged when `args` already carry the target flag or ask for `-h` / `--help`, or outside a git work tree; a `hide-global-config` install carrying `--force` or `--reset-hooks-path` still gets the override, so neither reaches the shared directory through the shim. Every other subcommand passes through to the real binary, the next `` on `PATH` that is not the shim. Exits 127 when the name it is installed under has no adapter or no real binary is found. | | Host home and run-time override | [`git-hook-dispatcher.sh`](../agent-isolation/git-hook-dispatcher.sh), from `~/.claude/git-hooks/` | Every hook git runs, before it chains to the repository's own hook | Outside the sandbox (no `SANDBOX_RUNTIME`), exports `=~/.local/share/` for every registered manager unless the operator's value lies outside `~/.cache`, and moves `XDG_CACHE_HOME` to `~/.local/share/prek-host/cache` by the same rule. Sandboxed hooks keep their values. Before chaining to a hook whose marker matches an adapter with `install.runtime_env`, exports the same `core.hooksPath` override the shim used, so the manager's own run-time check accepts the hook. | | Home check | [`sandbox-lint`](../sandbox-lint/) | `uv run --project tools/sandbox-lint sandbox-lint` | Fails a settings file whose `permissions.deny` lacks `Edit(/**)` for any registered manager, and, for a project that uses a manager (see [Detection](#detection)), one whose `env.` is missing or overlaps the default home. | | Registry sync | [`tools/dev/sync-hook-managers.py`](../dev/sync-hook-managers.py) | `tools/dev/sync-hook-managers.py [--check]`; the `sync-hook-managers` prek hook | Validates `registry.json`, rewrites the generated blocks of the three scripts above, and checks the committed settings and the sandbox-lint baseline. Exits 1 when a block changed (re-stage and commit), or, with `--check`, when one would. | @@ -179,6 +185,7 @@ What a repository or an operator configures is where each manager's environments | `cargo-husky` | Rust | `none`; detected by a hook mentioning `cargo-husky` | none (no external home) | shipping (in linked worktrees `build.rs` skips hook install: searches for a `.git` directory) | | `git-build-hook` | Java | `none`; detected by `pom.xml` (nested too) or a hook mentioning `git-build-hook` | none (no external home) | shipping (JGit `getGitDir()` targets `/hooks` in linked worktrees; `configure` goal can set repo-local `core.hooksPath`) | | `git-code-format-maven-plugin` | Java | `none`; detected by `pom.xml` (nested too) or a hook mentioning `git-code-format-maven-plugin` | none (no external home) | shipping (JGit `getCommonDirectory()` targets common hooks dir, coexisting cleanly in linked worktrees) | +| `lefthook` | Go | `hide-global-config`; detected by `lefthook.yml`, `lefthook.yaml`, `lefthook.json`, `lefthook.jsonc`, `lefthook.toml`, their `.lefthook` and `-local` variants, or a hook mentioning `call_lefthook` | none: remote configs live under `/lefthook-remotes`, inside `.git` | shipping | | `prek` | Rust | `git-dir-flag` (`--git-dir`); detected by `prek.toml`, `.pre-commit-config.yaml` or `.pre-commit-config.yml` (nested too) or a hook mentioning `prek` | `PREK_HOME`: `~/.cache/prek` → sandbox `~/.cache/prek-sandbox`, host `~/.local/share/prek-host` | shipping | | `overcommit` | Ruby | `hooks-path-env` (`--install`, alias `-i`, found anywhere in argv); detected by `.overcommit.yml` or a hook mentioning `overcommit` | none — it keeps its own state in git config and the repository | shipping | | `simple-git-hooks` | JavaScript | `none`; detected by `package.json`, `.simple-git-hooks.{js,cjs,json}`, `simple-git-hooks.{js,cjs,json}` or a hook mentioning `simple-git-hooks` | none (no external home) | shipping | @@ -204,13 +211,18 @@ What a repository or an operator configures is where each manager's environments `AbstractMavenGitCodeFormatMojo.getOrCreateHooksDirectory()` uses `gitRepository().getCommonDirectory().toPath().resolve(HOOKS_DIR)`. Because it targets the common directory, hooks land in `$(git rev-parse --git-common-dir)/hooks/` and coexist cleanly in linked worktrees. Installs a bash wrapper invoking `mvn git-code-format-maven-plugin:format-code`. + - `lefthook` (v2.2.2, verified against the release binary through the shim): + Refuses a bare install while a scoped `git config --global` read returns any `core.hooksPath`; the shim's `GIT_CONFIG_GLOBAL=/dev/null` hides it, and the install then writes to `$(git rev-parse --git-path hooks)`, the common hooks directory. + `--force` and `--reset-hooks-path` still get the override through the shim, so neither reaches the shared directory or unsets the setup. + In a linked worktree the same install lands in the main checkout's `.git/hooks/`. + npm's `postinstall` runs `node_modules/.bin/lefthook` ahead of `~/.claude/bin`, so the shim does not cover it: that install refuses, and the repository needs one run through the shim. + A fix would be the session-start hook noticing hooks in the shared directory that are not the dispatcher; the shim cannot reach an npm-run child ahead of it on `PATH`. - **JGit in linked worktrees (Maven):** The linked-worktree limitation applies only to `git-build-hook` (`InstallMojo`), which uses `getGitDir()` and creates hooks under `/worktrees//hooks/`. Because C-git commits read hooks exclusively from `$(git rev-parse --git-common-dir)/hooks/`, hooks installed by `git-build-hook` from within a worktree are ignored by Git. Conversely, `git-code-format-maven-plugin` explicitly calls `getCommonDirectory()`, so its hooks populate the common directory and function across worktrees. - **Gradle & Go hook tools:** - Standard Gradle plugins and Go hook utilities remain unadapted and unregistered until someone tests them under the dispatcher. - `lefthook` (Go) refuses install when a global `core.hooksPath` is set and is reserved for the planned `hide-global-config` install strategy. + Standard Gradle plugins and other Go hook utilities remain unadapted and unregistered until someone tests them under the dispatcher. ### Verified runs diff --git a/tools/hook-manager/registry.json b/tools/hook-manager/registry.json index 19b9b9a11..39ee8acc5 100644 --- a/tools/hook-manager/registry.json +++ b/tools/hook-manager/registry.json @@ -41,6 +41,40 @@ "strategy": "none" } }, + "lefthook": { + "language": "Go", + "upstream": "https://github.com/evilmartians/lefthook", + "detect": { + "config_files": [ + "lefthook.yml", + "lefthook.yaml", + "lefthook.json", + "lefthook.jsonc", + "lefthook.toml", + ".lefthook.yml", + ".lefthook.yaml", + ".lefthook.json", + ".lefthook.jsonc", + ".lefthook.toml", + "lefthook-local.yml", + "lefthook-local.yaml", + "lefthook-local.json", + "lefthook-local.jsonc", + "lefthook-local.toml", + ".lefthook-local.yml", + ".lefthook-local.yaml", + ".lefthook-local.json", + ".lefthook-local.jsonc", + ".lefthook-local.toml" + ], + "nested": false, + "hook_marker": "call_lefthook" + }, + "install": { + "subcommand": "install", + "strategy": "hide-global-config" + } + }, "prek": { "language": "Rust", "upstream": "https://github.com/j178/prek", diff --git a/tools/spec-loop/specs/agent-isolation-sandbox.md b/tools/spec-loop/specs/agent-isolation-sandbox.md index 68046e5b2..333d7f2df 100644 --- a/tools/spec-loop/specs/agent-isolation-sandbox.md +++ b/tools/spec-loop/specs/agent-isolation-sandbox.md @@ -364,6 +364,12 @@ See `docs/adapters/goose.md`. `tools/agent-isolation/hook-manager-shim.sh` (installed as `~/.claude/bin/`, keyed by that name), the dispatcher and `sandbox-lint`, and checks the committed settings against it. + The shim's per-adapter install strategy keeps the target local: prek + gets `--git-dir`, and lefthook, which refuses while a global + `core.hooksPath` is set, gets `GIT_CONFIG_GLOBAL=/dev/null` and has no + home (#1822). + The `lefthook-hooks-path` agent guard blocks `lefthook install --force` + and `--reset-hooks-path` while that path is the framework's. The settings template likewise denies `Edit(~/.cache/ponymail-attachments/**)` (#1828): the PonyMail MCP saves report attachments there from outside the sandbox, and a sandboxed command that could rename directories under that @@ -382,9 +388,10 @@ See `docs/adapters/goose.md`. shim, verify check 19d; the default-home deny and the dispatcher's host home stay on for every registered manager, because both homes are per user (#1819). - `registry.json` registers prek (install strategy `git-dir-flag`) and + `registry.json` registers prek (install strategy `git-dir-flag`), overcommit (`hooks-path-env`, with `install.runtime_env` so the - dispatcher exports the same `core.hooksPath` at run time), plus four + dispatcher exports the same `core.hooksPath` at run time), lefthook + (`hide-global-config`, no home), plus four managers with install strategy `none`, which already write to the repository's own hooks directory whatever the global `core.hooksPath`: `simple-git-hooks`, `cargo-husky`, `git-build-hook` and