fix: review-gate Stop hook output format and test harness
- Change exit code to 2 (Claude Code blocking signal) - Output JSON to stderr with decision: block and reason fields - Add integration test harness for live Claude testing - Update unit tests for new JSON output format - Add hooks.json example for Stop hook Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
dan
parent
4130dd4614
commit
020458c9b8
16
.claude/hooks.json
Normal file
16
.claude/hooks.json
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
{
|
||||
"hooks": {
|
||||
"Stop": [
|
||||
{
|
||||
"matcher": "",
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "/home/dan/proj/skills/skills/review-gate/scripts/review-gate check",
|
||||
"timeout": 30
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
|
|
@ -59,27 +59,24 @@ cmd_check() {
|
|||
exit 0
|
||||
;;
|
||||
pending)
|
||||
echo "⏳ BLOCKED: Review pending for session: $session_id"
|
||||
echo ""
|
||||
echo "To proceed, spawn a reviewer agent:"
|
||||
echo " /task Review the work in this session. Check git diff and file changes."
|
||||
echo " Then run: review-gate approve $session_id"
|
||||
echo ""
|
||||
echo "Or to skip review: review-gate approve $session_id"
|
||||
exit 1
|
||||
# Exit 2 + JSON to stderr = block in Claude Code Stop hook
|
||||
local reason="Review pending for session: $session_id. Spawn a reviewer agent with: /task Review the work. Check git diff and file changes. Then run: review-gate approve $session_id"
|
||||
jq -n --arg reason "$reason" '{
|
||||
"decision": "block",
|
||||
"reason": $reason
|
||||
}' >&2
|
||||
exit 2
|
||||
;;
|
||||
rejected)
|
||||
local reason=$(jq -r '.reason // "No reason provided"' "$state_file")
|
||||
local issues=$(jq -r '.issues[]? // empty' "$state_file" | head -5)
|
||||
echo "❌ BLOCKED: Review rejected for session: $session_id"
|
||||
echo "Reason: $reason"
|
||||
if [[ -n "$issues" ]]; then
|
||||
echo "Issues:"
|
||||
echo "$issues" | sed 's/^/ - /'
|
||||
fi
|
||||
echo ""
|
||||
echo "Fix the issues and request re-review."
|
||||
exit 1
|
||||
local reject_reason=$(jq -r '.reason // "No reason provided"' "$state_file")
|
||||
local issues=$(jq -r '.issues | join(", ")' "$state_file" 2>/dev/null || echo "")
|
||||
# Exit 2 + JSON to stderr = block in Claude Code Stop hook
|
||||
local reason="Review rejected for session: $session_id. Reason: $reject_reason. Issues: $issues. Fix the issues and request re-review."
|
||||
jq -n --arg reason "$reason" '{
|
||||
"decision": "block",
|
||||
"reason": $reason
|
||||
}' >&2
|
||||
exit 2
|
||||
;;
|
||||
*)
|
||||
echo "⚠ Unknown review status: $status"
|
||||
|
|
|
|||
224
skills/review-gate/tests/test-hook-integration.sh
Executable file
224
skills/review-gate/tests/test-hook-integration.sh
Executable file
|
|
@ -0,0 +1,224 @@
|
|||
#!/usr/bin/env bash
|
||||
# Integration test for review-gate Stop hook
|
||||
#
|
||||
# This test verifies that the Stop hook actually blocks Claude Code
|
||||
# when a review is pending.
|
||||
#
|
||||
# Usage:
|
||||
# ./test-hook-integration.sh [--debug]
|
||||
#
|
||||
# Requirements:
|
||||
# - Claude CLI installed
|
||||
# - API credits available
|
||||
# - Run from the skills repo root (or set SKILLS_ROOT)
|
||||
|
||||
set -uo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
SKILLS_ROOT="${SKILLS_ROOT:-$(cd "$SCRIPT_DIR/../../.." && pwd)}"
|
||||
REVIEW_GATE="$SKILLS_ROOT/skills/review-gate/scripts/review-gate"
|
||||
DEBUG="${1:-}"
|
||||
|
||||
# Colors
|
||||
RED='\033[0;31m'
|
||||
GREEN='\033[0;32m'
|
||||
YELLOW='\033[0;33m'
|
||||
NC='\033[0m'
|
||||
|
||||
# Test state (isolated)
|
||||
TEST_STATE_DIR=$(mktemp -d)
|
||||
TEST_HOOKS_DIR=$(mktemp -d)
|
||||
export REVIEW_STATE_DIR="$TEST_STATE_DIR"
|
||||
|
||||
cleanup() {
|
||||
rm -rf "$TEST_STATE_DIR" "$TEST_HOOKS_DIR"
|
||||
}
|
||||
trap cleanup EXIT
|
||||
|
||||
echo "=== Review Gate Hook Integration Test ==="
|
||||
echo "Skills root: $SKILLS_ROOT"
|
||||
echo "State dir: $TEST_STATE_DIR"
|
||||
echo ""
|
||||
|
||||
# --- Setup ---
|
||||
echo "## Setup"
|
||||
|
||||
# Create hooks.json that points to our review-gate
|
||||
# Use the test state dir via environment variable
|
||||
cat > "$TEST_HOOKS_DIR/hooks.json" <<EOF
|
||||
{
|
||||
"hooks": {
|
||||
"Stop": [
|
||||
{
|
||||
"matcher": "",
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "REVIEW_STATE_DIR=$TEST_STATE_DIR $REVIEW_GATE check",
|
||||
"timeout": 30
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
EOF
|
||||
|
||||
echo "Created test hooks.json"
|
||||
cat "$TEST_HOOKS_DIR/hooks.json"
|
||||
echo ""
|
||||
|
||||
# Enable a review with a fixed session ID
|
||||
# We'll set REVIEW_SESSION_ID in the hook command doesn't help because
|
||||
# the hook doesn't know the session. Let's use a predictable session ID.
|
||||
TEST_SESSION="integration-test-$(date +%s)"
|
||||
"$REVIEW_GATE" enable "$TEST_SESSION"
|
||||
echo "Enabled review for session: $TEST_SESSION"
|
||||
echo ""
|
||||
|
||||
# Verify the state
|
||||
echo "State file contents:"
|
||||
cat "$TEST_STATE_DIR/$TEST_SESSION.json"
|
||||
echo ""
|
||||
echo ""
|
||||
|
||||
# --- Test 1: Hook blocks when review pending ---
|
||||
echo "## Test 1: Hook script blocks when pending"
|
||||
|
||||
output=$("$REVIEW_GATE" check "$TEST_SESSION" 2>&1)
|
||||
exit_code=$?
|
||||
|
||||
if [[ $exit_code -eq 2 ]]; then
|
||||
echo -e "${GREEN}PASS${NC}: Exit code is 2 (blocking)"
|
||||
else
|
||||
echo -e "${RED}FAIL${NC}: Exit code is $exit_code (expected 2)"
|
||||
fi
|
||||
|
||||
if echo "$output" | grep -q '"decision": "block"'; then
|
||||
echo -e "${GREEN}PASS${NC}: Output contains decision: block"
|
||||
else
|
||||
echo -e "${RED}FAIL${NC}: Output missing decision: block"
|
||||
echo "Output was: $output"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
|
||||
# --- Test 2: Hook allows when approved ---
|
||||
echo "## Test 2: Hook script allows when approved"
|
||||
|
||||
"$REVIEW_GATE" approve "$TEST_SESSION" > /dev/null
|
||||
output=$("$REVIEW_GATE" check "$TEST_SESSION" 2>&1)
|
||||
exit_code=$?
|
||||
|
||||
if [[ $exit_code -eq 0 ]]; then
|
||||
echo -e "${GREEN}PASS${NC}: Exit code is 0 (allowing)"
|
||||
else
|
||||
echo -e "${RED}FAIL${NC}: Exit code is $exit_code (expected 0)"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
|
||||
# --- Test 3: Live Claude integration (optional) ---
|
||||
echo "## Test 3: Live Claude integration"
|
||||
echo ""
|
||||
echo "This test runs Claude with the Stop hook and verifies blocking."
|
||||
echo "It requires API credits and takes ~10-30 seconds."
|
||||
echo ""
|
||||
|
||||
# Re-enable review for live test
|
||||
"$REVIEW_GATE" enable "$TEST_SESSION" > /dev/null
|
||||
|
||||
# Check if we should run the live test
|
||||
if [[ "${SKIP_LIVE_TEST:-}" == "1" ]]; then
|
||||
echo -e "${YELLOW}SKIP${NC}: SKIP_LIVE_TEST=1 set, skipping live test"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
echo "Running: claude -p 'Say hello' with Stop hook..."
|
||||
echo ""
|
||||
|
||||
# Build the claude command
|
||||
# We need to inject our hooks somehow. Options:
|
||||
# 1. Use --mcp-config (doesn't help for hooks)
|
||||
# 2. Run from a directory with .claude/hooks.json
|
||||
# 3. Use debug mode to see what happens
|
||||
|
||||
# Create a temp project dir with our hooks
|
||||
TEST_PROJECT=$(mktemp -d)
|
||||
mkdir -p "$TEST_PROJECT/.claude"
|
||||
cp "$TEST_HOOKS_DIR/hooks.json" "$TEST_PROJECT/.claude/hooks.json"
|
||||
|
||||
# Also need a way to pass REVIEW_STATE_DIR to the hook
|
||||
# Rewrite hooks.json with absolute paths and env vars baked in
|
||||
cat > "$TEST_PROJECT/.claude/hooks.json" <<EOF
|
||||
{
|
||||
"hooks": {
|
||||
"Stop": [
|
||||
{
|
||||
"matcher": "",
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "REVIEW_STATE_DIR=$TEST_STATE_DIR REVIEW_SESSION_ID=$TEST_SESSION $REVIEW_GATE check",
|
||||
"timeout": 30
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
EOF
|
||||
|
||||
echo "Test project: $TEST_PROJECT"
|
||||
echo "Hooks config:"
|
||||
cat "$TEST_PROJECT/.claude/hooks.json"
|
||||
echo ""
|
||||
|
||||
# Run claude from the test project directory
|
||||
cd "$TEST_PROJECT"
|
||||
|
||||
if [[ "$DEBUG" == "--debug" ]]; then
|
||||
debug_flag="--debug hooks"
|
||||
else
|
||||
debug_flag=""
|
||||
fi
|
||||
|
||||
# Run with timeout to avoid hanging
|
||||
echo "Executing claude..."
|
||||
set +e
|
||||
output=$(timeout 60 claude -p "Say 'hello' and nothing else" $debug_flag 2>&1)
|
||||
exit_code=$?
|
||||
set -e
|
||||
|
||||
echo "Exit code: $exit_code"
|
||||
echo "Output:"
|
||||
echo "$output"
|
||||
echo ""
|
||||
|
||||
# Cleanup test project
|
||||
rm -rf "$TEST_PROJECT"
|
||||
|
||||
# Analyze results
|
||||
echo "## Analysis"
|
||||
|
||||
if echo "$output" | grep -qi "block"; then
|
||||
echo -e "${GREEN}PASS${NC}: Output contains 'block' - hook appears to have fired"
|
||||
elif echo "$output" | grep -qi "review pending"; then
|
||||
echo -e "${GREEN}PASS${NC}: Output contains 'review pending' - hook fired"
|
||||
elif echo "$output" | grep -qi "credit"; then
|
||||
echo -e "${YELLOW}SKIP${NC}: API credit issue - cannot complete live test"
|
||||
elif [[ $exit_code -eq 124 ]]; then
|
||||
echo -e "${YELLOW}SKIP${NC}: Timeout - Claude may be blocked in a loop (which could mean hook is working)"
|
||||
else
|
||||
echo -e "${YELLOW}INCONCLUSIVE${NC}: Could not determine if hook fired"
|
||||
echo "Check output above for clues"
|
||||
fi
|
||||
|
||||
echo ""
|
||||
echo "=== Test Complete ==="
|
||||
echo ""
|
||||
echo "To re-run live test when credits available:"
|
||||
echo " $0"
|
||||
echo ""
|
||||
echo "To skip live test:"
|
||||
echo " SKIP_LIVE_TEST=1 $0"
|
||||
|
|
@ -167,9 +167,9 @@ echo "## Check When Pending (Should Block)"
|
|||
run_gate enable test-pending > /dev/null
|
||||
output=$(run_gate check test-pending)
|
||||
exit_code=$?
|
||||
assert_exit_code "Check returns 1 when pending" "1" "$exit_code"
|
||||
assert_output_contains "Check shows BLOCKED" "BLOCKED" "$output"
|
||||
assert_output_contains "Check shows pending status" "pending" "$output"
|
||||
assert_exit_code "Check returns 2 when pending (blocks hook)" "2" "$exit_code"
|
||||
assert_output_contains "Check outputs decision: block" '"decision": "block"' "$output"
|
||||
assert_output_contains "Check reason mentions pending" "pending" "$output"
|
||||
|
||||
# --- Approve Command ---
|
||||
echo ""
|
||||
|
|
@ -206,9 +206,9 @@ echo "## Check When Rejected (Should Block)"
|
|||
|
||||
output=$(run_gate check test-reject)
|
||||
exit_code=$?
|
||||
assert_exit_code "Check returns 1 when rejected" "1" "$exit_code"
|
||||
assert_output_contains "Check shows BLOCKED" "BLOCKED" "$output"
|
||||
assert_output_contains "Check shows reason" "Missing tests" "$output"
|
||||
assert_exit_code "Check returns 2 when rejected (blocks hook)" "2" "$exit_code"
|
||||
assert_output_contains "Check outputs decision: block" '"decision": "block"' "$output"
|
||||
assert_output_contains "Check reason mentions rejection reason" "Missing tests" "$output"
|
||||
|
||||
# --- Reject With Issues ---
|
||||
echo ""
|
||||
|
|
@ -288,7 +288,7 @@ assert_json_field "Status reset to pending" "pending" ".status" "$TEST_STATE_DIR
|
|||
|
||||
output=$(run_gate check reset-session)
|
||||
exit_code=$?
|
||||
assert_exit_code "Check blocks after re-enable" "1" "$exit_code"
|
||||
assert_exit_code "Check blocks after re-enable" "2" "$exit_code"
|
||||
|
||||
# --- Clean Command ---
|
||||
echo ""
|
||||
|
|
|
|||
Loading…
Reference in a new issue