diff --git a/hooks/pretool-plan-gate.py b/hooks/pretool-plan-gate.py index 61f5894f..aea6c3b1 100755 --- a/hooks/pretool-plan-gate.py +++ b/hooks/pretool-plan-gate.py @@ -3,19 +3,25 @@ """ PreToolUse:Write,Edit Hook: Plan Gate -Blocks implementation when task_plan.md doesn't exist in the project root. +Blocks implementation when task_plan.md doesn't exist at the project root. Forces agents to create a plan before writing implementation code. This is a HARD GATE — exits 0 with JSON permissionDecision:deny to block the Write/Edit tool. Detection logic: - Tool is Write or Edit -- Target path is in agents/, skills/ -- task_plan.md does not exist in the project root +- Target path is under agents/ or skills/ — gated regardless of file extension, + because SKILL.md and agent .md files are behavioral specs (implementation, not docs) +- task_plan.md does not exist at the project root + +Project root resolution (Defect 1 fix): the plan is anchored to a stable +project root, not the session pwd (which is often a deep subdirectory). +Order: CLAUDE_PROJECT_DIR env → nearest ancestor of cwd containing .git → cwd. Allow-through conditions: -- Target file is NOT in agents/, skills/ -- task_plan.md exists in the project root +- Target file is NOT under agents/ or skills/ +- The target file is itself named task_plan.md (never gate the plan file) +- task_plan.md exists at the project root - PLAN_GATE_BYPASS=1 env var (for use by the plans skill itself) """ @@ -31,8 +37,8 @@ _BYPASS_ENV = "PLAN_GATE_BYPASS" -# Paths that ARE implementation code — only these get gated. -# Everything else (docs, config, CI, plans, tests, scripts, hooks) passes through. +# Paths that ARE gated — all files under these get gated regardless of extension, +# because SKILL.md and agent .md files are behavioral specs (implementation, not docs). _GATED_PREFIXES = ( "/agents/", "/skills/", @@ -45,6 +51,21 @@ def _is_gated(file_path: str) -> bool: return any(prefix in normalised for prefix in _GATED_PREFIXES) +def _find_project_root(event_cwd: str) -> Path: + """Resolve a stable project root to anchor task_plan.md. + + Order: CLAUDE_PROJECT_DIR env → nearest ancestor of cwd containing .git → cwd. + """ + env = os.environ.get("CLAUDE_PROJECT_DIR") + if env: + return Path(env).resolve() + cur = Path(event_cwd or ".").resolve() + for p in [cur, *cur.parents]: + if (p / ".git").exists(): + return p + return cur + + def main() -> None: debug = os.environ.get("CLAUDE_HOOKS_DEBUG") @@ -68,16 +89,22 @@ def main() -> None: if not file_path: sys.exit(0) - # Only gate implementation code (agents/, skills/, pipelines/). - # Everything else (docs, config, CI, plans, tests, scripts, hooks) passes through. + # Never gate the plan file itself — it is the unblocker (Defect 2). + if Path(file_path).name == "task_plan.md": + if debug: + print("[plan-gate] Target is task_plan.md — allowing through", file=sys.stderr) + sys.exit(0) + + # Gate all files under agents/ or skills/ regardless of extension, because + # SKILL.md and agent .md files are behavioral specs. Everything else allows. if not _is_gated(file_path): if debug: print(f"[plan-gate] Not a gated path, allowing: {file_path}", file=sys.stderr) sys.exit(0) - # Resolve project root: prefer event["cwd"], then CLAUDE_PROJECT_DIR, then cwd. - cwd_str = event.get("cwd") or os.environ.get("CLAUDE_PROJECT_DIR", ".") - base_dir = Path(cwd_str).resolve() + # Resolve a stable project root to anchor the plan (Defect 1): + # CLAUDE_PROJECT_DIR → nearest ancestor with .git → cwd. + base_dir = _find_project_root(event.get("cwd", ".")) plan_path = base_dir / "task_plan.md" if plan_path.is_file(): diff --git a/tests/test_plan_gate_hook.py b/tests/test_plan_gate_hook.py new file mode 100644 index 00000000..88a3b461 --- /dev/null +++ b/tests/test_plan_gate_hook.py @@ -0,0 +1,201 @@ +#!/usr/bin/env python3 +"""Tests for pretool-plan-gate.py — the plan-gate hook. + +TDD: each test was written BEFORE its fix and verified to FAIL on the +unfixed code, then PASS after the fix. Covers three defects: +- Defect 1: project-root resolution (walk up to .git), so a repo-root + task_plan.md satisfies the gate regardless of session pwd depth. +- Defect 2: the plan file itself (task_plan.md) is never gated. +- Defect 3: docstring/comments accurately state all agents//skills/ files + (including .md) are gated. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +# Resolve paths +REPO_ROOT = Path(__file__).resolve().parent.parent +HOOKS_DIR = REPO_ROOT / "hooks" +HOOK = HOOKS_DIR / "pretool-plan-gate.py" + + +def _run(event: dict, env_extra: dict | None = None): + """Invoke the hook with a synthetic event on stdin; return CompletedProcess.""" + env = os.environ.copy() + # Ensure a clean slate — no inherited bypass / project-dir override. + env.pop("PLAN_GATE_BYPASS", None) + env.pop("CLAUDE_PROJECT_DIR", None) + if env_extra: + env.update(env_extra) + return subprocess.run( + [sys.executable, str(HOOK)], + input=json.dumps(event), + capture_output=True, + text=True, + env=env, + timeout=10, + ) + + +def _is_deny(result) -> bool: + """A deny prints JSON with permissionDecision:deny to stdout.""" + stdout = result.stdout.strip() + if not stdout: + return False + try: + parsed = json.loads(stdout) + except json.JSONDecodeError: + return False + hso = parsed.get("hookSpecificOutput", {}) + return hso.get("permissionDecision") == "deny" + + +def _make_git_repo(tmp_path: Path) -> Path: + """Create a directory that looks like a git root (has .git).""" + (tmp_path / ".git").mkdir() + return tmp_path + + +# --------------------------------------------------------------------------- +# Acceptance 3: gated path, no plan at git root -> deny +# --------------------------------------------------------------------------- + + +def test_gated_path_no_plan_denies(tmp_path): + root = _make_git_repo(tmp_path) + skill_file = root / "skills" / "x" / "SKILL.md" + skill_file.parent.mkdir(parents=True) + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Write", + "tool_input": {"file_path": str(skill_file)}, + "cwd": str(root), + } + result = _run(event) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert _is_deny(result), f"Expected deny, got stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Acceptance 1 / Defect 1: plan at git root, deep cwd -> allow +# --------------------------------------------------------------------------- + + +def test_plan_at_git_root_deep_cwd_allows(tmp_path): + root = _make_git_repo(tmp_path) + (root / "task_plan.md").write_text("plan\n") + deep = root / "skills" / "engineering" / "opensearch-api-client" / "scripts" + deep.mkdir(parents=True) + skill_file = root / "skills" / "engineering" / "opensearch-api-client" / "SKILL.md" + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Edit", + "tool_input": {"file_path": str(skill_file)}, + "cwd": str(deep), + } + result = _run(event) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert not _is_deny(result), f"Expected allow (plan at git root, deep cwd), got deny. stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Acceptance 2 / Defect 2: writing task_plan.md is never gated +# --------------------------------------------------------------------------- + + +def test_writing_plan_file_in_gated_subtree_allows(tmp_path): + root = _make_git_repo(tmp_path) + # No task_plan.md at root yet — the point is to create one under skills/. + target = root / "skills" / "x" / "task_plan.md" + target.parent.mkdir(parents=True) + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Write", + "tool_input": {"file_path": str(target)}, + "cwd": str(root), + } + result = _run(event) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert not _is_deny(result), f"task_plan.md must never be gated. stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Acceptance 4: non-gated path -> allow +# --------------------------------------------------------------------------- + + +def test_non_gated_path_allows(tmp_path): + root = _make_git_repo(tmp_path) + doc = root / "docs" / "x.md" + doc.parent.mkdir(parents=True) + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Write", + "tool_input": {"file_path": str(doc)}, + "cwd": str(root), + } + result = _run(event) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert not _is_deny(result), f"Non-gated path must allow. stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Acceptance 5: PLAN_GATE_BYPASS=1 -> allow +# --------------------------------------------------------------------------- + + +def test_bypass_env_allows(tmp_path): + root = _make_git_repo(tmp_path) + skill_file = root / "skills" / "x" / "SKILL.md" + skill_file.parent.mkdir(parents=True) + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Write", + "tool_input": {"file_path": str(skill_file)}, + "cwd": str(root), + } + result = _run(event, env_extra={"PLAN_GATE_BYPASS": "1"}) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert not _is_deny(result), f"PLAN_GATE_BYPASS=1 must allow. stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Defect 1 (env override): CLAUDE_PROJECT_DIR takes precedence +# --------------------------------------------------------------------------- + + +def test_claude_project_dir_env_used_for_root(tmp_path): + root = tmp_path / "proj" # no .git — force use of the env var + (root / "skills" / "x").mkdir(parents=True) + (root / "task_plan.md").write_text("plan\n") + skill_file = root / "skills" / "x" / "SKILL.md" + event = { + "hook_event_name": "PreToolUse", + "tool_name": "Write", + "tool_input": {"file_path": str(skill_file)}, + "cwd": str(root / "skills" / "x"), + } + result = _run(event, env_extra={"CLAUDE_PROJECT_DIR": str(root)}) + assert result.returncode == 0, f"Hook exited non-zero: {result.stderr}" + assert not _is_deny(result), f"CLAUDE_PROJECT_DIR should anchor root. stdout={result.stdout!r}" + + +# --------------------------------------------------------------------------- +# Defect 3: docstring/comments accurately describe gating scope +# --------------------------------------------------------------------------- + + +def test_docstring_states_all_files_gated(): + source = HOOK.read_text() + lowered = source.lower() + # Misleading "passes through" wording for docs/config/CI/tests must be gone. + assert "passes through" not in lowered, "Misleading 'passes through' wording still present" + # Must state gating is regardless of extension / behavioral spec rationale. + assert "regardless of extension" in lowered or "behavioral spec" in lowered, ( + "Docstring/comments do not state that all agents//skills/ files are gated" + )