diff --git a/.cursor/rules/development-process.mdc b/.cursor/rules/development-process.mdc index a38f005..2a1f72a 100644 --- a/.cursor/rules/development-process.mdc +++ b/.cursor/rules/development-process.mdc @@ -82,8 +82,9 @@ Record the chosen tier and rationale in the PR description. Receipt: after both reviews finish, write `.cursor/review-receipts/pre-commit.json` (gitignored) via the - review-bugbot / review-security skills so the pre-commit shell hook - can allow `git commit`. Do not bypass with `--no-verify`. + review-bugbot / review-security skills. Enable native enforcement + once per clone with `make hooks-install` (`core.hooksPath=.githooks`). + Do not bypass with `--no-verify`. 6. **Strongest model only.** Implementation and review subagents run on the strongest available model. If a subagent silently falls back to diff --git a/.githooks/README.md b/.githooks/README.md new file mode 100644 index 0000000..f71c297 --- /dev/null +++ b/.githooks/README.md @@ -0,0 +1,26 @@ +# Native git hooks (review receipt) + +This directory is intended for `core.hooksPath` (not the Python +[`pre-commit`](https://pre-commit.com) framework in `.pre-commit-config.yaml`). + +## Enable (per clone) + +```bash +make hooks-install +# equivalent: git config core.hooksPath .githooks +``` + +`pre-commit` runs `python3 scripts/record_review_receipt.py --check`, then +(if the `pre-commit` CLI is installed) chains into the Python +[pre-commit](https://pre-commit.com) framework via `pre-commit hook-impl` so +`core.hooksPath=.githooks` does not disable `.pre-commit-config.yaml`. + +## Framework hooks + +If `.pre-commit-config.yaml` exists, this hook requires a resolvable +`pre-commit` CLI (`PATH`, `.venv/bin/pre-commit`, or `python3 -m pre_commit`) +and chains `pre-commit hook-impl`. Missing CLI → commit fails with a warning +(so `core.hooksPath` cannot silently drop framework checks). + +- `ROUNDTABLE_SKIP_REVIEW_RECEIPT=1` — emergency skip (audited locally) +- `git commit --no-verify` — still possible; process forbids it for Roundtable work diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000..f41200d --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# Native pre-commit: require Bugbot + Security Review receipt for this tree, +# then run the Python pre-commit framework hooks (if available) so setting +# core.hooksPath=.githooks does not disable .pre-commit-config.yaml. +# Enable with: make hooks-install +set -euo pipefail + +ROOT="$(git rev-parse --show-toplevel)" +cd "$ROOT" + +skip="${ROUNDTABLE_SKIP_REVIEW_RECEIPT:-}" +case "$(printf '%s' "$skip" | tr '[:upper:]' '[:lower:]')" in + 1|true|yes) + echo "ROUNDTABLE_SKIP_REVIEW_RECEIPT set; skipping review receipt check." + ;; + *) + SCRIPT="$ROOT/scripts/record_review_receipt.py" + if [[ ! -f "$SCRIPT" ]]; then + echo "record_review_receipt.py missing; cannot verify review receipt." >&2 + exit 1 + fi + python3 "$SCRIPT" --check + ;; +esac + +# Preserve framework hooks that normally live under .git/hooks when +# core.hooksPath points at this directory instead. +if [[ ! -f "$ROOT/.pre-commit-config.yaml" ]]; then + exit 0 +fi + +run_hook_impl() { + local bin="$1" + shift + exec "$bin" hook-impl \ + --config="$ROOT/.pre-commit-config.yaml" \ + --hook-type=pre-commit \ + --hook-dir="$ROOT/.githooks" \ + -- "$@" +} + +if command -v pre-commit >/dev/null 2>&1; then + run_hook_impl pre-commit "$@" +fi +if [[ -x "$ROOT/.venv/bin/pre-commit" ]]; then + run_hook_impl "$ROOT/.venv/bin/pre-commit" "$@" +fi +if python3 -c "import pre_commit" >/dev/null 2>&1; then + exec python3 -m pre_commit hook-impl \ + --config="$ROOT/.pre-commit-config.yaml" \ + --hook-type=pre-commit \ + --hook-dir="$ROOT/.githooks" \ + -- "$@" +fi + +echo "WARNING: .pre-commit-config.yaml is present but the pre-commit CLI was" >&2 +echo "not found on PATH, in .venv/bin, or via python3 -m pre_commit." >&2 +echo "Framework hooks (quick checks / secret scan) were NOT run." >&2 +echo "Activate your venv or: pip install pre-commit && pre-commit install-hooks" >&2 +exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cde9ba0..6be7c0a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,10 +4,11 @@ This repo follows a gated review workflow — the full version lives in [docs/DE 1. **Plan first.** Open an issue or draft describing the change. Non-trivial work produces the design artifacts required by the process doc (architecture map, data flow, wireframes or workflow states) and gets a design review (security, operations, and template-DX perspectives as relevant) before implementation starts. Tests are planned before production logic. 2. **Branch and keep the PR focused.** One change per PR; no direct commits to `main`. -3. **Validate before requesting review.** `bash scripts/validate_generated.sh` must pass completely — it generates a project from the template and runs the full 18-check pipeline (tests, lint, security scans, red-team checks, unrendered-template guard, the injection-defense golden set). The script exits 0 even when it prints warnings; read the output and resolve or justify warnings in the PR. -4. **Update docs in the same PR.** Capability claims cite code and tests; any quoted test/check counts must come from an actual run. Limitations belong in the GOVERNANCE Non-Claims section — never overclaim. -5. **Review findings get regression tests.** A fix should include a test that fails on the pre-fix code. -6. **Squash-merge after approval and green CI.** +3. **Install local review hooks once per clone:** `make hooks-install` (sets `core.hooksPath=.githooks` so commits require a Bugbot + Security receipt). +4. **Validate before requesting review.** `bash scripts/validate_generated.sh` must pass completely — it generates a project from the template and runs the full 18-check pipeline (tests, lint, security scans, red-team checks, unrendered-template guard, the injection-defense golden set). The script exits 0 even when it prints warnings; read the output and resolve or justify warnings in the PR. +5. **Update docs in the same PR.** Capability claims cite code and tests; any quoted test/check counts must come from an actual run. Limitations belong in the GOVERNANCE Non-Claims section — never overclaim. +6. **Review findings get regression tests.** A fix should include a test that fails on the pre-fix code. +7. **Squash-merge after approval and green CI.** Design constraints worth knowing before you write code: diff --git a/Makefile b/Makefile index 8e8b5cc..51c9747 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ # # Run 'make help' to see all targets. -.PHONY: help quick validate validate-matrix fix clean +.PHONY: help quick validate validate-matrix fix clean hooks-install TEMPLATE_DIR := template/{{project_slug}} TEMPLATE_SRC := $(TEMPLATE_DIR)/src/{{project_slug}} @@ -20,6 +20,12 @@ help: ## Show all available targets @echo "================================" @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-20s\033[0m %s\n", $$1, $$2}' +hooks-install: ## Point this clone at .githooks (review-receipt pre-commit) + @chmod +x .githooks/pre-commit + @git config core.hooksPath .githooks + @echo "core.hooksPath=.githooks (native review-receipt pre-commit enabled)" + @echo "Note: chains to Python pre-commit framework when that CLI is installed." + # ============================================================================= # FAST CHECKS (~5 seconds, run on templates directly) # ============================================================================= diff --git a/docs/DEVELOPMENT_PROCESS.md b/docs/DEVELOPMENT_PROCESS.md index d001e00..f6e7567 100644 --- a/docs/DEVELOPMENT_PROCESS.md +++ b/docs/DEVELOPMENT_PROCESS.md @@ -108,12 +108,13 @@ still applies. Low-tier changes preserve: `.cursor/review-receipts/pre-commit.json`). Autofix stays off unless a maintainer explicitly enables it. - **Non-claim:** The Cursor `beforeShellExecution` receipt gate is - **defense-in-depth** for lone `git commit` in Cursor. It is not - cryptographic proof reviews ran, not complete against wrappers / - merge / cherry-pick / non-Cursor terminals, and must not be described - as fail-closed commit integrity until a native git hook follow-up - lands (see `docs/designs/pre-commit-review-gate/THREAT_MODEL.md`). + **Receipt gate:** Cursor `beforeShellExecution` is **defense-in-depth**. + Enable the native hook once per clone with `make hooks-install` + (`core.hooksPath=.githooks`) so `pre-commit` re-checks the receipt for + any `git commit` (including terminal, merge/cherry-pick, and many + wrappers). Still not cryptographic proof; `--no-verify` and + `ROUNDTABLE_SKIP_REVIEW_RECEIPT` remain local bypasses — see + `docs/designs/pre-commit-review-gate/THREAT_MODEL.md`. The maintainer records the tier and rationale in the PR description whenever invoking the Low exemption. diff --git a/docs/designs/pre-commit-review-gate/DESIGN_NOTE.md b/docs/designs/pre-commit-review-gate/DESIGN_NOTE.md index f56a5e4..cac1503 100644 --- a/docs/designs/pre-commit-review-gate/DESIGN_NOTE.md +++ b/docs/designs/pre-commit-review-gate/DESIGN_NOTE.md @@ -8,7 +8,7 @@ Require Bugbot **and** Security Review on the current worktree before any `git commit` to this Roundtable checkout, via a Cursor `beforeShellExecution` hook and a gitignored receipt file. -**Honest posture:** defense-in-depth for Cursor `git commit`, not fail-closed integrity. See `THREAT_MODEL.md`. Native `core.hooksPath` / `pre-commit` is the follow-up that closes residual bypasses. +**Honest posture:** Cursor gate is defense-in-depth; native `.githooks/pre-commit` via `make hooks-install` is the commit-time receipt check. See `THREAT_MODEL.md` and `NATIVE_HOOKS.md`. ## Split from Task ISA diff --git a/docs/designs/pre-commit-review-gate/NATIVE_HOOKS.md b/docs/designs/pre-commit-review-gate/NATIVE_HOOKS.md new file mode 100644 index 0000000..2dc6914 --- /dev/null +++ b/docs/designs/pre-commit-review-gate/NATIVE_HOOKS.md @@ -0,0 +1,50 @@ +# Design delta: Native pre-commit hooks + +**Risk-tier:** High (follow-up to `pre-commit-review-gate`) +**Branch:** `feat/native-pre-commit-hooks` +**Depends on:** receipt script + Cursor gate from `feat/pre-commit-review-gate` + +## Intent + +Install a repo-managed git hook via `core.hooksPath=.githooks` that runs +`scripts/record_review_receipt.py --check` at commit time. This closes the +Cursor-only and compound-TOCTOU gaps documented in `THREAT_MODEL.md` for the +defense-in-depth Cursor gate. + +## Architecture impact + +```text +git commit (any client) + → .githooks/pre-commit + → record_review_receipt.py --check + → allow / deny (exit code) +``` + +Cursor `beforeShellExecution` remains defense-in-depth (early deny, clearer +agent messages). Native hook is the integrity check that applies outside Cursor. + +## Data movement + +Unchanged fingerprint/receipt path. Hook cwd is the committing worktree +toplevel (`git rev-parse --show-toplevel` inside the hook). + +## Failure behavior + +- Missing/stale receipt → commit aborted (exit 1) +- Missing script → exit 1 +- `ROUNDTABLE_SKIP_REVIEW_RECEIPT` → allow with stdout notice +- `--no-verify` → residual (documented) + +## Risks + +| Risk | Handling | +|------|----------| +| Developers forget `make hooks-install` | Document in CONTRIBUTING + DEVELOPMENT_PROCESS; CI cannot set local hooksPath | +| Confusion with Python `pre-commit` framework | `.githooks/` name + README; hook chains `pre-commit hook-impl` when CLI present | +| `core.hooksPath` would skip framework hooks | Chain `pre-commit hook-impl` after receipt check | +| Merge/cherry-pick without reviews | Hook runs; may block until receipt matches post-merge tree (honest friction) | + +## Planned tests + +- Unit: hook script invokes `--check` and respects skip env (subprocess) +- Existing fingerprint / gate tests remain authoritative for receipt logic diff --git a/docs/designs/pre-commit-review-gate/THREAT_MODEL.md b/docs/designs/pre-commit-review-gate/THREAT_MODEL.md index 5fd9118..5eb10bd 100644 --- a/docs/designs/pre-commit-review-gate/THREAT_MODEL.md +++ b/docs/designs/pre-commit-review-gate/THREAT_MODEL.md @@ -1,37 +1,37 @@ -# Threat Model: Pre-commit Review Gate +# Threat Model: Pre-commit Review Gate (+ native hooks) **Asset:** Integrity of the Roundtable git history — commits should not land without Bugbot + Security Review on the same tree. **Attacker:** Local developer or coding agent with shell access to the checkout (trusted-developer threat model; not remote multi-tenant). -**Posture:** This PR is **defense-in-depth for Cursor shell `git commit`**, not fail-closed commit integrity. Native git hooks are a required follow-up. +**Posture:** Cursor `beforeShellExecution` is **defense-in-depth**. With `core.hooksPath=.githooks` enabled (`make hooks-install`), the native `pre-commit` hook re-checks the receipt at commit time for any client that honors git hooks. ## Abuse cases and mitigations -| ID | Abuse case | Severity | Mitigation in this PR | Residual | -|----|------------|----------|----------------------|----------| -| T1 | Skip reviews, lone `git commit` / `git -C` / `git -c` in Cursor | High | Hook (no brittle matcher) denies without receipt | — | -| T2 | Compound `… && git commit` (TOCTOU) | High | Compound / multi-`git` commands **deny** (fail closed); `-m` text tokenized so messages are not false denials | Mutations still possible via wrappers; native hook re-checks at commit time | -| T3 | Stage v1, edit worktree to v2, commit index | High | `--check` requires no unstaged tracked diffs; fingerprint is path→bytes (staging-stable) | — | -| T4 | `git -C otherrepo commit` using this receipt | High | Enforce only when `--git-common-dir` matches Roundtable | — | -| T5 | Linked worktree ≠ hook install path | High | Compare `--git-common-dir`; run `--check` with `cwd=target` toplevel | — | -| T6 | `git --git-dir=…` / glued `-Cpath` / `GIT_DIR=` / `GIT_WORK_TREE=` env forms | Medium | Unsupported forms **deny** | — | -| T7 | Self-attested receipt without real reviews | Medium | Documented; skills + process require honest recording | Any local actor can forge receipt | -| T8 | `python -c` / wrapper invoking `git commit` | Medium | Not covered by Cursor shell hook argv | **Native `.git/hooks/pre-commit` follow-up** | -| T9 | Commit outside Cursor (plain terminal) | Medium | Project hook only fires in Cursor | Native git hook follow-up | -| T10 | `--no-verify` | Low/Medium | Cursor hook still sees `git commit` | Terminal without Cursor | -| T11 | `ROUNDTABLE_SKIP_REVIEW_RECEIPT=1` | Accepted | Documented emergency bypass | Human audit | -| T12 | Invalid hook JSON / crash | High | Fail-closed deny / exit 2 with `failClosed: true` | — | -| T13 | `git merge` / `cherry-pick` / `rebase --continue` | High | Not treated as `git commit` by gate | Native hook / broader matchers follow-up | -| T14 | `git` alias hiding `commit` | Medium | Tokenization looks for `commit` subcommand | Native hook follow-up | +| ID | Abuse case | Severity | Mitigation | Residual | +|----|------------|----------|------------|----------| +| T1 | Skip reviews, lone `git commit` / `git -C` / `git -c` in Cursor | High | Cursor hook + native `pre-commit` `--check` | — when hooksPath set | +| T2 | Compound `… && git commit` (TOCTOU) | High | Cursor denies compound; native hook re-checks **after** mutations at commit | — when hooksPath set | +| T3 | Stage v1, edit worktree to v2, commit index | High | `--check` requires no unstaged tracked diffs; fingerprint path→bytes | — | +| T4 | `git -C otherrepo commit` using this receipt | High | Cursor enforces git-common-dir; native hook uses committing toplevel | — | +| T5 | Linked worktree ≠ hook install path | High | Cursor `cwd=target`; native hook uses that worktree toplevel | — | +| T6 | `git --git-dir=…` / glued `-Cpath` / `GIT_DIR=` / `GIT_WORK_TREE=` | Medium | Cursor denies unsupported forms | Exotic env still possible if hooks skipped | +| T7 | Self-attested receipt without real reviews | Medium | Documented; skills + process | Any local actor can forge receipt | +| T8 | `python -c` / wrapper invoking `git commit` | Medium | **Native hook** runs if wrapper still calls `git commit` | Wrappers that bypass git hooks | +| T9 | Commit outside Cursor (plain terminal) | Medium | **Native hook** when hooksPath installed | Clone without `make hooks-install` | +| T10 | `--no-verify` | Low/Medium | Process forbids | Still bypasses native hook | +| T11 | `ROUNDTABLE_SKIP_REVIEW_RECEIPT=1` | Accepted | Documented emergency bypass (Cursor + native) | Human audit | +| T12 | Invalid Cursor hook JSON / crash | High | Fail-closed deny / exit 2 | — | +| T13 | `git merge` / `cherry-pick` / `rebase --continue` | High | **Native pre-commit** runs on resulting commit | Tree may need fresh reviews after merge | +| T14 | `git` alias hiding `commit` | Medium | Native hook still runs on the commit | Cursor may miss alias forms | ## Non-goals - Cryptographic proof that Bugbot/Security models ran. - Replacing CI (`validate_generated.sh`, Gitleaks, pip-audit). - Multi-user attestation. -- Claiming fail-closed integrity against a shell-capable agent (Phase 3 native hooks). +- Forcing `core.hooksPath` via committed git config (must be per-clone `make hooks-install`). -## Follow-up (required for enforcement claims) +## Install expectation -Ship a native `pre-commit` (or `core.hooksPath`) that calls `scripts/record_review_receipt.py --check` so T8/T9/T13 close regardless of Cursor, and compound TOCTOU is re-checked at commit time. +Contributors run `make hooks-install` once per clone. CI and hosts that never commit locally are unaffected. diff --git a/tests/test_pre_commit_review_gate.py b/tests/test_pre_commit_review_gate.py index fb39f9c..ad10d14 100644 --- a/tests/test_pre_commit_review_gate.py +++ b/tests/test_pre_commit_review_gate.py @@ -130,5 +130,43 @@ def test_hooks_json_has_no_matcher(self) -> None: self.assertNotIn("matcher", entry) +class NativePreCommitHook(unittest.TestCase): + HOOK = REPO_ROOT / ".githooks" / "pre-commit" + + def test_hook_is_executable_and_calls_check(self) -> None: + self.assertTrue(self.HOOK.is_file()) + text = self.HOOK.read_text(encoding="utf-8") + self.assertIn("record_review_receipt.py", text) + self.assertIn("--check", text) + self.assertIn("ROUNDTABLE_SKIP_REVIEW_RECEIPT", text) + self.assertIn("hook-impl", text) + self.assertIn(".venv/bin/pre-commit", text) + + def test_skip_env_exits_zero_without_framework_config(self) -> None: + import os + import shutil + + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + subprocess.check_call(["git", "init"], cwd=root) + hooks = root / ".githooks" + hooks.mkdir() + dest = hooks / "pre-commit" + shutil.copy(self.HOOK, dest) + dest.chmod(0o755) + env = os.environ.copy() + env["ROUNDTABLE_SKIP_REVIEW_RECEIPT"] = "1" + proc = subprocess.run( + ["bash", str(dest)], + cwd=str(root), + env=env, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(proc.returncode, 0) + self.assertIn("skipping", proc.stdout.lower()) + + if __name__ == "__main__": unittest.main()