From dd2911da1c50f1d9ca8d4e1036149ef2e7c62092 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Fri, 16 Oct 2026 15:14:46 +0700 Subject: [PATCH] feat(hook): pre-commit hook v2 accepts a validated diff and upgrades in place (#674) Signed-off-by: ajianaz --- src/hook/install.rs | 33 +++- src/hook/template.rs | 100 ++++++++--- tests/commit_gate.rs | 400 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 508 insertions(+), 25 deletions(-) create mode 100644 tests/commit_gate.rs diff --git a/src/hook/install.rs b/src/hook/install.rs index 2881dc3..1728271 100644 --- a/src/hook/install.rs +++ b/src/hook/install.rs @@ -6,6 +6,15 @@ use crate::hook::template::HOOK_TEMPLATE; /// Sentinel marker identifying a cora-managed hook. const CORA_HOOK_SENTINEL: &str = "# cora-managed-hook"; +/// First line of the v1 hook, which had no sentinel. Hooks carrying it are cora's +/// own and are upgraded in place instead of being wrapped a second time. +const LEGACY_HOOK_MARK: &str = "cora pre-commit hook — installed by `cora hook install`"; + +/// Is `content` a hook written by `cora hook install` (any version)? +fn is_cora_managed(content: &str) -> bool { + content.contains(CORA_HOOK_SENTINEL) || content.contains(LEGACY_HOOK_MARK) +} + /// Install the pre-commit hook to `.git/hooks/pre-commit`. pub fn install_hook() -> Result { let hooks_dir = find_git_hooks_dir()?; @@ -14,7 +23,7 @@ pub fn install_hook() -> Result { // Check if a hook already exists and handle accordingly if hook_path.is_file() { let existing = std::fs::read_to_string(&hook_path)?; - if existing.contains(CORA_HOOK_SENTINEL) { + if is_cora_managed(&existing) { // Already a cora-managed hook — just overwrite debug!("existing hook is cora-managed, overwriting"); } else { @@ -139,6 +148,7 @@ pub fn is_hook_installed() -> Result { #[cfg(test)] mod tests { + use super::is_cora_managed; use std::fs; use std::process::Command; @@ -187,4 +197,25 @@ mod tests { "install.rs should consistently use '{expected_backup}' for backup" ); } + + use crate::hook::template::HOOK_TEMPLATE; + + #[test] + fn current_and_legacy_hooks_are_recognised_as_cora_managed() { + assert!(is_cora_managed(HOOK_TEMPLATE)); + let v1 = "#!/usr/bin/env bash\n# cora pre-commit hook — installed by `cora hook install`\n# Run `cora hook uninstall` to remove.\n"; + assert!(is_cora_managed(v1), "v1 hooks had no sentinel"); + let wrapped = format!( + "#!/bin/sh\necho mine\n\n# cora-managed-hook — the section below was added by `cora hook install`\n{HOOK_TEMPLATE}" + ); + assert!(is_cora_managed(&wrapped)); + } + + #[test] + fn a_users_own_hook_is_not_cora_managed() { + assert!(!is_cora_managed("#!/bin/sh\nnpm test\n")); + assert!(!is_cora_managed( + "#!/bin/sh\n# runs cora review by hand\ncora review --staged\n" + )); + } } diff --git a/src/hook/template.rs b/src/hook/template.rs index 8ef8d88..71d2a60 100644 --- a/src/hook/template.rs +++ b/src/hook/template.rs @@ -1,38 +1,90 @@ -/// Pre-commit hook template content. +/// Pre-commit hook template content (v2, #674). /// -/// The hook runs `cora index --quiet` (incremental, ~0.014s) then -/// `cora review --staged --format compact` and checks the exit -/// code: 0 = ok (allow commit), 1 = error (allow commit), 2 = blocked (deny commit). +/// 1. A staged diff that an agent (`cora.review_finalize`) or `cora review +/// --staged` already judged passes at once: `cora hook verify` exits 0. +/// 2. Otherwise the hook runs `cora review --staged --ci --severity major` +/// (an LLM when a key is configured, the deterministic checks otherwise). +/// +/// `cora review` exit codes: 0 = pass, 2 = blocking findings (deny the commit), +/// anything else = tooling error (the commit proceeds, but it is NOT a pass). +/// `cora hook verify` exit codes: 0 validated, 10 validated but blocking, +/// 11 not validated; an older cora without the subcommand exits 2 (a usage +/// error), which falls through to the full review. pub const HOOK_TEMPLATE: &str = r#"#!/usr/bin/env bash -# cora pre-commit hook — installed by `cora hook install` +# cora-managed-hook +# cora pre-commit hook (v2) — installed by `cora hook install`. # Run `cora hook uninstall` to remove. -set -euo pipefail +set -uo pipefail # Locate cora binary (prefer the one used to install the hook) CORA_BIN="${CORA_BIN:-cora}" -echo "🔍 Running cora code review..." +# 1. The staged diff was already reviewed (by an agent or by `cora review --staged`). +"$CORA_BIN" hook verify --quiet +case $? in + 0) + echo "✅ cora: staged diff already validated." + exit 0 + ;; + 10) + echo "❌ cora: the staged diff was reviewed and the verdict blocks the commit." + echo " Fix the findings, git add, and review again (see: cora hook verify)." + exit 2 + ;; +esac +# 11 = not validated; any other code (an older cora) falls through to a full review. + +# 2. Fallback: review the staged diff now. +echo "🔍 Running cora review..." # Update the symbol index (incremental — fast, non-blocking) -"$CORA_BIN" index 2>/dev/null || true - -# Run cora review on staged changes (uses index for brain enrichment) -if "$CORA_BIN" review --staged --format compact 2>/dev/null; then - echo "✅ cora review passed." - exit 0 -else - EXIT_CODE=$? - if [ $EXIT_CODE -eq 2 ]; then +"$CORA_BIN" index >/dev/null 2>&1 || true + +"$CORA_BIN" review --staged --ci --severity major --format compact +code=$? +case $code in + 0) + echo "✅ cora review passed." + exit 0 + ;; + 2) echo "❌ cora review found blocking issues. Commit denied." - echo " Fix the issues above, or run: git commit --no-verify" + echo " Fix the issues above; do not use --no-verify to get past a finding." exit 2 - elif [ $EXIT_CODE -eq 1 ]; then - echo "⚠️ cora review encountered an error (commit will proceed)." + ;; + *) + echo "⚠️ cora review did not complete (exit $code): that is not a pass." + echo " Re-run: cora review --base --ci --severity major" + echo " and get a real verdict before pushing." exit 0 - else - echo "⚠️ cora exited with code $EXIT_CODE (commit will proceed)." - exit 0 - fi -fi + ;; +esac "#; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn template_is_a_managed_v2_hook_with_both_tiers() { + assert!(HOOK_TEMPLATE.starts_with("#!/usr/bin/env bash\n# cora-managed-hook\n")); + assert!(HOOK_TEMPLATE.contains("hook verify --quiet")); + assert!(HOOK_TEMPLATE.contains("review --staged --ci --severity major")); + } + + #[test] + fn template_denies_only_on_exit_2_or_10() { + // The tooling-error branch must not deny (and must say it is no pass). + assert!(HOOK_TEMPLATE.contains("that is not a pass")); + assert_eq!(HOOK_TEMPLATE.matches("exit 2").count(), 2); + // An old cora (usage error 2 from `hook verify`) falls through. + assert!(HOOK_TEMPLATE.contains("falls through to a full review")); + } + + #[test] + fn template_never_tells_users_to_bypass_a_finding() { + assert!(HOOK_TEMPLATE.contains("do not use --no-verify to get past a finding")); + assert!(!HOOK_TEMPLATE.contains("or run: git commit --no-verify")); + } +} diff --git a/tests/commit_gate.rs b/tests/commit_gate.rs new file mode 100644 index 0000000..dc6f0dd --- /dev/null +++ b/tests/commit_gate.rs @@ -0,0 +1,400 @@ +//! End-to-end tests of the commit gate (#674) against real git repositories: +//! `cora hook verify`, `cora hook claude-guard`, and the installed pre-commit hook. +//! cora runs without any API key (deterministic judge), so nothing touches the network. + +use assert_cmd::cargo::cargo_bin; +use std::io::Write; +use std::path::Path; +use std::process::{Command, Stdio}; + +struct Repo { + dir: tempfile::TempDir, + home: tempfile::TempDir, +} + +impl Repo { + fn new() -> Self { + let r = Repo { + dir: tempfile::tempdir().unwrap(), + home: tempfile::tempdir().unwrap(), + }; + r.git(&["init", "-q"]); + r.git(&["config", "user.email", "t@example.test"]); + r.git(&["config", "user.name", "t"]); + std::fs::write(r.dir.path().join("base.py"), "x = 1\n").unwrap(); + r.git(&["add", "-A"]); + r.git(&["commit", "-q", "--no-verify", "-m", "init"]); + r + } + + fn path(&self) -> &Path { + self.dir.path() + } + + fn git(&self, args: &[&str]) -> std::process::Output { + Command::new("git") + .args(args) + .current_dir(self.path()) + .env("HOME", self.home.path()) + .output() + .unwrap() + } + + /// A cora invocation with no API key and an isolated HOME. + fn cora(&self, args: &[&str]) -> Command { + let mut c = Command::new(cargo_bin("cora")); + c.args(args) + .current_dir(self.path()) + .env_clear() + .env("PATH", "/usr/bin:/bin") + .env("HOME", self.home.path()); + c + } + + fn stage(&self, file: &str, content: &str) { + std::fs::write(self.path().join(file), content).unwrap(); + self.git(&["add", file]); + } + + fn verify_code(&self) -> i32 { + self.cora(&["hook", "verify", "--quiet"]) + .status() + .unwrap() + .code() + .unwrap() + } + + fn guard(&self, stdin_json: &str) -> (i32, String) { + let mut child = self + .cora(&["hook", "claude-guard"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + child + .stdin + .take() + .unwrap() + .write_all(stdin_json.as_bytes()) + .unwrap(); + let out = child.wait_with_output().unwrap(); + ( + out.status.code().unwrap(), + String::from_utf8_lossy(&out.stderr).to_string(), + ) + } +} + +fn bash(command: &str) -> String { + serde_json::json!({ "tool_name": "Bash", "tool_input": { "command": command } }).to_string() +} + +const CLEAN: &str = "def add(a, b):\n return a + b\n"; +const SECRET: &str = "password = \"hunter2hunter2\"\n"; + +#[test] +fn nothing_staged_is_nothing_to_gate() { + let r = Repo::new(); + assert_eq!(r.verify_code(), 0); +} + +#[test] +fn an_unreviewed_staged_diff_is_not_validated_until_it_is_reviewed() { + let r = Repo::new(); + r.stage("util.py", CLEAN); + assert_eq!(r.verify_code(), 11, "never reviewed"); + + // A deterministic review of the staged diff records the verdict. + let out = r + .cora(&["review", "--staged", "--format", "compact"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(r.verify_code(), 0, "reviewed and clean"); + + // Editing and re-staging invalidates it. + r.stage("util.py", "def add(a, b):\n return a - b\n"); + assert_eq!(r.verify_code(), 11, "edited since the review"); +} + +#[test] +fn a_blocking_verdict_blocks_the_gate() { + let r = Repo::new(); + r.stage("app.py", SECRET); + let _ = r + .cora(&["review", "--staged", "--format", "compact"]) + .output() + .unwrap(); + assert_eq!(r.verify_code(), 10, "the hardcoded secret blocks"); +} + +#[test] +fn the_claude_guard_denies_unreviewed_commits_and_allows_reviewed_ones() { + let r = Repo::new(); + r.stage("util.py", CLEAN); + + let (code, why) = r.guard(&bash("git commit -m 'add util'")); + assert_eq!(code, 2); + assert!(why.contains("has not been reviewed"), "{why}"); + + r.cora(&["review", "--staged", "--format", "compact"]) + .output() + .unwrap(); + assert_eq!(r.guard(&bash("git commit -m 'add util'")).0, 0); +} + +#[test] +fn the_claude_guard_never_allows_no_verify() { + let r = Repo::new(); + r.stage("util.py", CLEAN); + r.cora(&["review", "--staged", "--format", "compact"]) + .output() + .unwrap(); + for cmd in [ + "git commit --no-verify -m x", + "git commit -nm x", + "git add . && git commit -n -m x", + ] { + let (code, why) = r.guard(&bash(cmd)); + assert_eq!(code, 2, "{cmd}"); + assert!(why.contains("not allowed"), "{why}"); + } +} + +#[test] +fn the_claude_guard_blocks_a_reviewed_but_blocking_diff() { + let r = Repo::new(); + r.stage("app.py", SECRET); + r.cora(&["review", "--staged", "--format", "compact"]) + .output() + .unwrap(); + let (code, why) = r.guard(&bash("git commit -m x")); + assert_eq!(code, 2); + assert!(why.contains("blocks the commit"), "{why}"); +} + +#[test] +fn the_claude_guard_leaves_everything_else_alone() { + let r = Repo::new(); + r.stage("util.py", CLEAN); // unreviewed, but these are not commits + assert_eq!(r.guard(&bash("git status")).0, 0); + assert_eq!(r.guard(&bash("cargo test")).0, 0); + assert_eq!(r.guard(&bash("git log --oneline")).0, 0); + let other_tool = serde_json::json!({ "tool_name": "Edit", "tool_input": {} }).to_string(); + assert_eq!(r.guard(&other_tool).0, 0); + assert_eq!(r.guard("not json at all").0, 0); + assert_eq!(r.guard("").0, 0); +} + +#[test] +fn the_installed_hook_blocks_a_secret_and_passes_clean_code() { + let r = Repo::new(); + let cora = cargo_bin("cora"); + let install = r.cora(&["hook", "install"]).output().unwrap(); + assert!( + install.status.success(), + "{}", + String::from_utf8_lossy(&install.stderr) + ); + let hook = std::fs::read_to_string(r.path().join(".git/hooks/pre-commit")).unwrap(); + assert!(hook.contains("# cora-managed-hook") && hook.contains("hook verify --quiet")); + + let commit = |msg: &str| { + Command::new("git") + .args(["commit", "-q", "-m", msg]) + .current_dir(r.path()) + .env_clear() + .env( + "PATH", + format!("/usr/bin:/bin:{}", cora.parent().unwrap().display()), + ) + .env("HOME", r.home.path()) + .env("CORA_BIN", &cora) + .output() + .unwrap() + }; + + // A secret: the fallback review denies the commit. + r.stage("app.py", SECRET); + let denied = commit("add secret"); + assert!( + !denied.status.success(), + "{}", + String::from_utf8_lossy(&denied.stdout) + ); + let log = r.git(&["log", "--oneline"]); + assert_eq!( + String::from_utf8_lossy(&log.stdout).lines().count(), + 1, + "no commit was made" + ); + + // Clean code: the hook passes (the review it runs also records the verdict). + r.git(&["reset", "-q"]); + r.stage("util.py", CLEAN); + let ok = commit("add util"); + assert!( + ok.status.success(), + "{}{}", + String::from_utf8_lossy(&ok.stdout), + String::from_utf8_lossy(&ok.stderr) + ); + let log = r.git(&["log", "--oneline"]); + assert_eq!(String::from_utf8_lossy(&log.stdout).lines().count(), 2); +} + +#[test] +fn reinstalling_the_hook_upgrades_it_instead_of_wrapping_it_again() { + let r = Repo::new(); + // A v1 hook, as written before the sentinel existed. + let v1 = "#!/usr/bin/env bash\n# cora pre-commit hook — installed by `cora hook install`\ncora review --staged --format compact\n"; + let path = r.path().join(".git/hooks/pre-commit"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, v1).unwrap(); + assert!( + r.cora(&["hook", "install"]) + .output() + .unwrap() + .status + .success() + ); + let after = std::fs::read_to_string(&path).unwrap(); + assert_eq!( + after.matches("hook verify --quiet").count(), + 1, + "one hook, not two" + ); + assert!( + !after.contains("cora review --staged --format compact\n#"), + "old body is gone" + ); + assert!( + !r.path().join(".git/hooks/pre-commit.pre-cora.bak").exists(), + "no wrapping backup" + ); +} + +/// Drive `cora mcp` over stdio: initialize, then one tools/call. Returns the +/// text of the tool result. +fn mcp_call(r: &Repo, tool: &str, arguments: serde_json::Value) -> String { + let msgs = [ + serde_json::json!({"jsonrpc":"2.0","id":0,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"t","version":"1"}}}), + serde_json::json!({"jsonrpc":"2.0","method":"notifications/initialized"}), + serde_json::json!({"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":tool,"arguments":arguments}}), + ]; + let mut child = r + .cora(&["mcp"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + { + let stdin = child.stdin.as_mut().unwrap(); + for m in &msgs { + writeln!(stdin, "{m}").unwrap(); + } + } + let out = child.wait_with_output().unwrap(); + let last = String::from_utf8_lossy(&out.stdout) + .lines() + .rfind(|l| l.contains("\"id\":1")) + .expect("tool response") + .to_string(); + let v: serde_json::Value = serde_json::from_str(&last).unwrap(); + v["result"]["content"][0]["text"] + .as_str() + .unwrap() + .to_string() +} + +fn staged_diff(r: &Repo) -> String { + String::from_utf8_lossy(&r.git(&["diff", "--cached"]).stdout).to_string() +} + +#[test] +fn an_agent_verdict_validates_the_exact_staged_diff_for_hook_and_guard() { + let r = Repo::new(); + r.stage("util.py", CLEAN); + assert_eq!(r.verify_code(), 11); + assert_eq!(r.guard(&bash("git commit -m x")).0, 2); + + // The agent judges (no findings of its own) and finalizes with the diff. + let text = mcp_call( + &r, + "cora.review_finalize", + serde_json::json!({ + "diff": staged_diff(&r), "issues": [] + }), + ); + let v: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(v["commit_gate"]["recorded"], true, "{text}"); + assert_eq!(v["commit_gate"]["blocking"], false); + + assert_eq!(r.verify_code(), 0, "validated by the agent"); + assert_eq!(r.guard(&bash("git commit -m x")).0, 0); + + // A different staged change is not covered by that verdict. + r.stage("other.py", "y = 2\n"); + assert_eq!(r.verify_code(), 11); + assert_eq!(r.guard(&bash("git commit -m x")).0, 2); +} + +#[test] +fn an_agent_verdict_with_a_blocking_finding_blocks_the_commit() { + let r = Repo::new(); + r.stage("util.py", CLEAN); + let text = mcp_call( + &r, + "cora.review_finalize", + serde_json::json!({ + "diff": staged_diff(&r), + "issues": [{"file":"util.py","line":2,"severity":"critical","title":"bad","body":"b"}] + }), + ); + let v: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(v["commit_gate"]["blocking"], true, "{text}"); + assert_eq!(r.verify_code(), 10); + assert_eq!(r.guard(&bash("git commit -m x")).0, 2); +} + +#[test] +fn the_hook_accepts_what_the_agent_already_validated_without_a_second_review() { + let r = Repo::new(); + let cora = cargo_bin("cora"); + assert!( + r.cora(&["hook", "install"]) + .output() + .unwrap() + .status + .success() + ); + r.stage("util.py", CLEAN); + mcp_call( + &r, + "cora.review_finalize", + serde_json::json!({"diff": staged_diff(&r), "issues": []}), + ); + let out = Command::new("git") + .args(["commit", "-q", "-m", "add util"]) + .current_dir(r.path()) + .env_clear() + // No `cora review` could run: only the verify step is reachable. + .env("PATH", "/usr/bin:/bin") + .env("HOME", r.home.path()) + .env("CORA_BIN", &cora) + .output() + .unwrap(); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + // git forwards a hook's stdout to its own stderr. + assert!(String::from_utf8_lossy(&out.stderr).contains("already validated")); +}