From e2b3064b76dcf30033d098a9494baca0289de47d Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 20:58:50 -0800 Subject: [PATCH 01/17] Refactor: General code quality improvements (Phase 1-5) Bug Fixes (Phase 1): - Fix sed patterns for Goal/AC extraction using POSIX [[:space:]] - Add numeric validation for line counts before comparison - Re-validate Codex model/effort at execution time - Add plan file readability check (-r) - Validate plan content has non-blank lines Test Coverage (Phase 2): - Add template edge case tests (empty vars, unicode) - Add command injection pattern tests (new test file) - Add plan file content validation tests - Add Python todo checker error handling tests (new test file) Code Simplification (Phase 3): - Add parse_state_file() shared function - Extract magic strings to constants (FIELD_*, MARKER_*, EXIT_*) - Cache git status output to avoid duplicate calls Edge Case Handling (Phase 4): - Improve goal tracker placeholder detection with generic regex - Validate existing git timeout wrapper (portable-timeout.sh) - Validate existing Python todo checker graceful error handling Documentation (Phase 5): - Add template variable syntax documentation - Document exit reasons with constants All tests pass. Version bumped to 1.1.5. --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- README.md | 2 +- hooks/lib/loop-common.sh | 78 +++++++ hooks/lib/template-loader.sh | 15 ++ hooks/loop-codex-stop-hook.sh | 55 +++-- scripts/setup-rlcr-loop.sh | 24 +- tests/test-bash-validator-patterns.sh | 214 ++++++++++++++++++ tests/test-plan-file-validation.sh | 79 +++++++ tests/test-template-loader.sh | 70 ++++++ tests/test-todo-checker.sh | 313 ++++++++++++++++++++++++++ 11 files changed, 835 insertions(+), 19 deletions(-) create mode 100755 tests/test-bash-validator-patterns.sh create mode 100755 tests/test-todo-checker.sh diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 36a2c6ae..9b44bd08 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ "name": "humanize", "source": "./", "description": "Humanize - An iterative development plugin that uses Codex to review Claude's work. Creates a feedback loop where Claude implements plans and Codex independently reviews progress, ensuring quality through continuous refinement.", - "version": "1.1.4" + "version": "1.1.5" } ] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 3838abeb..1b9b2294 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "humanize", "description": "Humanize - An iterative development plugin that uses Codex to review Claude's work. Creates a feedback loop where Claude implements plans and Codex independently reviews progress, ensuring quality through continuous refinement.", - "version": "1.1.4", + "version": "1.1.5", "author": { "name": "humania-org" }, diff --git a/README.md b/README.md index 1d5b557b..a536cefc 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Humanize -**Current Version: 1.1.4** +**Current Version: 1.1.5** > Derived from the [GAAC (GitHub-as-a-Context)](https://github.com/SihaoLiu/gaac) project. diff --git a/hooks/lib/loop-common.sh b/hooks/lib/loop-common.sh index eba9607f..78434069 100755 --- a/hooks/lib/loop-common.sh +++ b/hooks/lib/loop-common.sh @@ -8,6 +8,41 @@ # - loop-bash-validator.sh # +# ======================================== +# Constants +# ======================================== + +# State file field names +readonly FIELD_PLAN_TRACKED="plan_tracked" +readonly FIELD_START_BRANCH="start_branch" +readonly FIELD_PLAN_FILE="plan_file" +readonly FIELD_CURRENT_ROUND="current_round" +readonly FIELD_MAX_ITERATIONS="max_iterations" +readonly FIELD_PUSH_EVERY_ROUND="push_every_round" +readonly FIELD_CODEX_MODEL="codex_model" +readonly FIELD_CODEX_EFFORT="codex_effort" +readonly FIELD_CODEX_TIMEOUT="codex_timeout" + +# Codex review markers +readonly MARKER_COMPLETE="COMPLETE" +readonly MARKER_STOP="STOP" + +# Exit reasons (used with end_loop function) +# complete - Codex confirmed all goals achieved (normal success) +# cancel - User cancelled with /cancel-rlcr-loop +# maxiter - Reached maximum iterations limit +# stop - Codex triggered circuit breaker (stagnation detected) +# unexpected - System error or invalid state (e.g., corrupted state file) +readonly EXIT_COMPLETE="complete" +readonly EXIT_CANCEL="cancel" +readonly EXIT_MAXITER="maxiter" +readonly EXIT_STOP="stop" +readonly EXIT_UNEXPECTED="unexpected" + +# ======================================== +# Library Setup +# ======================================== + # Source template loader LOOP_COMMON_DIR="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)" source "$LOOP_COMMON_DIR/template-loader.sh" @@ -57,6 +92,49 @@ get_current_round() { echo "${current_round:-0}" } +# Parse state file frontmatter and set variables +# Usage: parse_state_file "$STATE_FILE" +# Sets the following variables (caller must declare them): +# STATE_FRONTMATTER - raw frontmatter content +# STATE_PLAN_TRACKED - "true" or "false" +# STATE_START_BRANCH - branch name +# STATE_PLAN_FILE - plan file path +# STATE_CURRENT_ROUND - current round number +# STATE_MAX_ITERATIONS - max iterations +# STATE_PUSH_EVERY_ROUND - "true" or "false" +# STATE_CODEX_MODEL - codex model name +# STATE_CODEX_EFFORT - codex effort level +# STATE_CODEX_TIMEOUT - codex timeout in seconds +# Returns: 0 on success, 1 if file not found +parse_state_file() { + local state_file="$1" + + if [[ ! -f "$state_file" ]]; then + return 1 + fi + + STATE_FRONTMATTER=$(sed -n '/^---$/,/^---$/{ /^---$/d; p; }' "$state_file" 2>/dev/null || echo "") + + # Parse fields with consistent quote handling + # Legacy quote-stripping kept for backward compatibility with older state files + STATE_PLAN_TRACKED=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_PLAN_TRACKED}:" | sed "s/${FIELD_PLAN_TRACKED}: *//" | tr -d ' ' || true) + STATE_START_BRANCH=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_START_BRANCH}:" | sed "s/${FIELD_START_BRANCH}: *//; s/^\"//; s/\"\$//" || true) + STATE_PLAN_FILE=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_PLAN_FILE}:" | sed "s/${FIELD_PLAN_FILE}: *//; s/^\"//; s/\"\$//" || true) + STATE_CURRENT_ROUND=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_CURRENT_ROUND}:" | sed "s/${FIELD_CURRENT_ROUND}: *//" | tr -d ' ' || true) + STATE_MAX_ITERATIONS=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_MAX_ITERATIONS}:" | sed "s/${FIELD_MAX_ITERATIONS}: *//" | tr -d ' ' || true) + STATE_PUSH_EVERY_ROUND=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_PUSH_EVERY_ROUND}:" | sed "s/${FIELD_PUSH_EVERY_ROUND}: *//" | tr -d ' ' || true) + STATE_CODEX_MODEL=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_CODEX_MODEL}:" | sed "s/${FIELD_CODEX_MODEL}: *//" | tr -d ' ' || true) + STATE_CODEX_EFFORT=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_CODEX_EFFORT}:" | sed "s/${FIELD_CODEX_EFFORT}: *//" | tr -d ' ' || true) + STATE_CODEX_TIMEOUT=$(echo "$STATE_FRONTMATTER" | grep "^${FIELD_CODEX_TIMEOUT}:" | sed "s/${FIELD_CODEX_TIMEOUT}: *//" | tr -d ' ' || true) + + # Apply defaults + STATE_CURRENT_ROUND="${STATE_CURRENT_ROUND:-0}" + STATE_MAX_ITERATIONS="${STATE_MAX_ITERATIONS:-10}" + STATE_PUSH_EVERY_ROUND="${STATE_PUSH_EVERY_ROUND:-false}" + + return 0 +} + # Convert a string to lowercase to_lower() { echo "$1" | tr '[:upper:]' '[:lower:]' diff --git a/hooks/lib/template-loader.sh b/hooks/lib/template-loader.sh index 7df141b0..7f46853d 100644 --- a/hooks/lib/template-loader.sh +++ b/hooks/lib/template-loader.sh @@ -3,7 +3,22 @@ # Template loading functions for RLCR loop hooks # # This library provides functions to load and render prompt templates. +# +# Template Variable Syntax +# ======================== # Templates use {{VARIABLE_NAME}} syntax for placeholders. +# - Variable names: uppercase letters, numbers, underscores only +# - Example: {{PLAN_FILE}}, {{CURRENT_ROUND}}, {{GOAL_TRACKER_FILE}} +# - Single-pass substitution: {{VAR}} in a value will NOT be expanded +# - Missing variables: placeholder is kept as-is (e.g., {{UNDEFINED}}) +# +# Available functions: +# - get_template_dir: Get path to template directory +# - load_template: Load a template file by name +# - render_template: Replace {{VAR}} placeholders with values +# - load_and_render: Load and render in one call +# - load_and_render_safe: Same as above but with fallback for missing templates +# - validate_template_dir: Check if template directory is valid # # Get the template directory path diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 18df65e6..420164be 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -93,6 +93,19 @@ CODEX_MODEL="${CODEX_MODEL:-$DEFAULT_CODEX_MODEL}" CODEX_EFFORT="${CODEX_EFFORT:-$DEFAULT_CODEX_EFFORT}" CODEX_TIMEOUT="${STATE_CODEX_TIMEOUT:-${CODEX_TIMEOUT:-$DEFAULT_CODEX_TIMEOUT}}" +# Re-validate Codex Model and Effort for YAML safety (in case state.md was manually edited) +# Use same validation patterns as setup-rlcr-loop.sh +if [[ ! "$CODEX_MODEL" =~ ^[a-zA-Z0-9._-]+$ ]]; then + echo "Error: Invalid codex_model in state file: $CODEX_MODEL" >&2 + end_loop "$LOOP_DIR" "$STATE_FILE" "unexpected" + exit 0 +fi +if [[ ! "$CODEX_EFFORT" =~ ^[a-zA-Z0-9_-]+$ ]]; then + echo "Error: Invalid codex_effort in state file: $CODEX_EFFORT" >&2 + end_loop "$LOOP_DIR" "$STATE_FILE" "unexpected" + exit 0 +fi + # Validate numeric fields early if [[ ! "$CURRENT_ROUND" =~ ^[0-9]+$ ]]; then echo "Warning: State file corrupted (current_round), stopping loop" >&2 @@ -253,6 +266,20 @@ Complete these tasks before exiting: fi fi +# ======================================== +# Cache Git Status Output +# ======================================== +# Cache git status output to avoid calling it multiple times. +# Used by both large file check and git clean check below. + +GIT_STATUS_CACHED="" +GIT_IS_REPO=false + +if command -v git &>/dev/null && git rev-parse --git-dir &>/dev/null 2>&1; then + GIT_IS_REPO=true + GIT_STATUS_CACHED=$(git status --porcelain 2>/dev/null || echo "") +fi + # ======================================== # Quick Check: Large File Detection # ======================================== @@ -261,7 +288,7 @@ fi MAX_LINES=2000 -if command -v git &>/dev/null && git rev-parse --git-dir &>/dev/null 2>&1; then +if [[ "$GIT_IS_REPO" == "true" ]]; then LARGE_FILES="" while IFS= read -r line; do @@ -303,13 +330,14 @@ if command -v git &>/dev/null && git rev-parse --git-dir &>/dev/null 2>&1; then # Count lines and trim whitespace (portable across shells) line_count=$(wc -l < "$filename" 2>/dev/null | tr -d ' ') || continue + # Validate line_count is numeric before comparison + [[ "$line_count" =~ ^[0-9]+$ ]] || continue + if [ "$line_count" -gt "$MAX_LINES" ]; then LARGE_FILES="${LARGE_FILES} - \`${filename}\`: ${line_count} lines (${file_type} file)" fi - done </dev/null) -EOF + done <<< "$GIT_STATUS_CACHED" if [ -n "$LARGE_FILES" ]; then FALLBACK="# Large Files Detected @@ -341,18 +369,17 @@ fi # Before running expensive Codex review, check if all changes have been # committed and pushed. This ensures work is properly saved. -# Check if git is available and we're in a git repo -if command -v git &>/dev/null && git rev-parse --git-dir &>/dev/null 2>&1; then +# Use cached git status from above +if [[ "$GIT_IS_REPO" == "true" ]]; then GIT_ISSUES="" SPECIAL_NOTES="" - # Check for uncommitted changes (staged or unstaged) - GIT_STATUS=$(git status --porcelain 2>/dev/null) - if [[ -n "$GIT_STATUS" ]]; then + # Check for uncommitted changes (staged or unstaged) using cached status + if [[ -n "$GIT_STATUS_CACHED" ]]; then GIT_ISSUES="uncommitted changes" # Check for special cases in untracked files - UNTRACKED=$(echo "$GIT_STATUS" | grep '^??' || true) + UNTRACKED=$(echo "$GIT_STATUS_CACHED" | grep '^??' || true) # Check if .humanize* directories are untracked (includes .humanize/ and any legacy .humanize-* dirs) if echo "$UNTRACKED" | grep -q '\.humanize'; then @@ -472,15 +499,17 @@ if [[ "$CURRENT_ROUND" -eq 0 ]] && [[ -f "$GOAL_TRACKER_FILE" ]]; then HAS_AC_PLACEHOLDER=false HAS_TASKS_PLACEHOLDER=false - if echo "$GOAL_TRACKER_CONTENT" | grep -q '\[To be extracted from plan'; then + # Use a generic placeholder pattern to detect uninitialized sections + # This matches "[To be extracted/defined/populated ..." patterns more robustly + if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*plan'; then HAS_GOAL_PLACEHOLDER=true fi - if echo "$GOAL_TRACKER_CONTENT" | grep -q '\[To be defined by Claude'; then + if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*Claude.*Round 0'; then HAS_AC_PLACEHOLDER=true fi - if echo "$GOAL_TRACKER_CONTENT" | grep -q '\[To be populated by Claude'; then + if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*Claude.*plan\]'; then HAS_TASKS_PLACEHOLDER=true fi diff --git a/scripts/setup-rlcr-loop.sh b/scripts/setup-rlcr-loop.sh index c1755a60..68ff2061 100755 --- a/scripts/setup-rlcr-loop.sh +++ b/scripts/setup-rlcr-loop.sh @@ -262,6 +262,12 @@ if [[ ! -f "$FULL_PLAN_PATH" ]]; then exit 1 fi +# Check file is readable +if [[ ! -r "$FULL_PLAN_PATH" ]]; then + echo "Error: Plan file not readable: $PLAN_FILE" >&2 + exit 1 +fi + # Check file is within project (no ../ escaping) # Resolve the real path by cd'ing to the directory and getting pwd # This handles symlinks in parent directories and ../ path components @@ -339,6 +345,16 @@ if [[ "$LINE_COUNT" -lt 5 ]]; then exit 1 fi +# Check plan has actual content (not just whitespace/blank lines) +# Exclude blank lines and lines that are only markdown comments () +NON_BLANK_LINES=$(grep -cvE '^[[:space:]]*$' "$FULL_PLAN_PATH" 2>/dev/null || echo "0") +if [[ "$NON_BLANK_LINES" -lt 3 ]]; then + echo "Error: Plan file has insufficient content (only $NON_BLANK_LINES non-blank lines)" >&2 + echo "" >&2 + echo "The plan file should contain meaningful content, not just blank lines." >&2 + exit 1 +fi + # Check codex is available if ! command -v codex &>/dev/null; then echo "Error: start-rlcr-loop requires codex to run" >&2 @@ -447,10 +463,11 @@ GOAL_TRACKER_EOF # Extract goal from plan file (look for ## Goal, ## Objective, or first paragraph) # This is a heuristic - Claude will refine it in round 0 -GOAL_LINE=$(grep -i -m1 '^\s*##\s*\(goal\|objective\|purpose\)' "$FULL_PLAN_PATH" 2>/dev/null || echo "") +# Use ^## without leading whitespace - markdown headers should start at column 0 +GOAL_LINE=$(grep -i -m1 '^##[[:space:]]*\(goal\|objective\|purpose\)' "$FULL_PLAN_PATH" 2>/dev/null || echo "") if [[ -n "$GOAL_LINE" ]]; then # Get the content after the heading - GOAL_SECTION=$(sed -n '/^\s*##\s*[Gg]oal\|^\s*##\s*[Oo]bjective\|^\s*##\s*[Pp]urpose/,/^\s*##/p' "$FULL_PLAN_PATH" | head -20 | tail -n +2 | head -10) + GOAL_SECTION=$(sed -n '/^##[[:space:]]*[Gg]oal\|^##[[:space:]]*[Oo]bjective\|^##[[:space:]]*[Pp]urpose/,/^##/p' "$FULL_PLAN_PATH" | head -20 | tail -n +2 | head -10) echo "$GOAL_SECTION" >> "$GOAL_TRACKER_FILE" else # Use first non-empty, non-heading paragraph as goal description @@ -468,7 +485,8 @@ cat >> "$GOAL_TRACKER_FILE" << 'GOAL_TRACKER_EOF' GOAL_TRACKER_EOF # Extract acceptance criteria from plan file (look for ## Acceptance, ## Criteria, ## Requirements) -AC_SECTION=$(sed -n '/^\s*##\s*[Aa]cceptance\|^\s*##\s*[Cc]riteria\|^\s*##\s*[Rr]equirements/,/^\s*##/p' "$FULL_PLAN_PATH" 2>/dev/null | head -30 | tail -n +2 | head -25) +# Use ^## without leading whitespace - markdown headers should start at column 0 +AC_SECTION=$(sed -n '/^##[[:space:]]*[Aa]cceptance\|^##[[:space:]]*[Cc]riteria\|^##[[:space:]]*[Rr]equirements/,/^##/p' "$FULL_PLAN_PATH" 2>/dev/null | head -30 | tail -n +2 | head -25) if [[ -n "$AC_SECTION" ]]; then echo "$AC_SECTION" >> "$GOAL_TRACKER_FILE" else diff --git a/tests/test-bash-validator-patterns.sh b/tests/test-bash-validator-patterns.sh new file mode 100755 index 00000000..62a491f7 --- /dev/null +++ b/tests/test-bash-validator-patterns.sh @@ -0,0 +1,214 @@ +#!/bin/bash +# +# Test script for command_modifies_file function in loop-common.sh +# +# Tests the regex patterns used to detect file modification commands +# to ensure proper blocking of file writes via Bash. +# + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +source "$PROJECT_ROOT/hooks/lib/loop-common.sh" + +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' # No Color + +TESTS_PASSED=0 +TESTS_FAILED=0 + +# Test helper functions +pass() { + echo -e "${GREEN}PASS${NC}: $1" + TESTS_PASSED=$((TESTS_PASSED + 1)) +} + +fail() { + echo -e "${RED}FAIL${NC}: $1" + echo " Command: $2" + echo " Expected: $3" + TESTS_FAILED=$((TESTS_FAILED + 1)) +} + +# Assert that a command SHOULD be detected as modifying the target file +assert_modifies() { + local command="$1" + local pattern="${2:-goal-tracker\\.md}" + local command_lower + command_lower=$(to_lower "$command") + + if command_modifies_file "$command_lower" "$pattern"; then + pass "Correctly detected modification: $command" + else + fail "Should detect modification" "$command" "should match pattern" + fi +} + +# Assert that a command should NOT be detected as modifying the target file +assert_not_modifies() { + local command="$1" + local pattern="${2:-goal-tracker\\.md}" + local command_lower + command_lower=$(to_lower "$command") + + if command_modifies_file "$command_lower" "$pattern"; then + fail "Should NOT detect modification" "$command" "should not match pattern" + else + pass "Correctly ignored: $command" + fi +} + +echo "========================================" +echo "Testing command_modifies_file patterns" +echo "========================================" +echo "" + +# ======================================== +# Test Group 1: Redirection operators (> >>) +# ======================================== +echo "Test Group 1: Redirection operators" +echo "" + +assert_modifies "echo x > goal-tracker.md" +assert_modifies "echo x >> goal-tracker.md" +assert_modifies "cat foo >> goal-tracker.md" +assert_modifies "printf 'text' > goal-tracker.md" +assert_modifies "echo 'data' > /path/to/goal-tracker.md" +assert_modifies "ECHO X > GOAL-TRACKER.MD" + +# ======================================== +# Test Group 2: tee command +# ======================================== +echo "" +echo "Test Group 2: tee command" +echo "" + +assert_modifies "tee goal-tracker.md" +assert_modifies "tee -a goal-tracker.md" +assert_modifies "echo x | tee goal-tracker.md" +assert_modifies "echo x | tee -a goal-tracker.md" +assert_modifies "cat file | tee /path/to/goal-tracker.md" + +# ======================================== +# Test Group 3: In-place editors (sed, awk, perl) +# ======================================== +echo "" +echo "Test Group 3: In-place editors" +echo "" + +assert_modifies "sed -i 's/x/y/' goal-tracker.md" +assert_modifies "sed -i.bak 's/x/y/' goal-tracker.md" +assert_modifies "sed -i '' 's/x/y/' goal-tracker.md" +assert_modifies "awk -i inplace '{print}' goal-tracker.md" +assert_modifies "perl -i -pe 's/x/y/' goal-tracker.md" +assert_modifies "perl -pie 's/x/y/' goal-tracker.md" + +# ======================================== +# Test Group 4: File operations (mv, cp, rm) +# ======================================== +echo "" +echo "Test Group 4: File operations" +echo "" + +assert_modifies "mv temp.md goal-tracker.md" +assert_modifies "cp backup.md goal-tracker.md" +assert_modifies "rm goal-tracker.md" +assert_modifies "rm -f goal-tracker.md" +assert_modifies "rm -rf goal-tracker.md" +assert_modifies "mv /tmp/new.md /path/to/goal-tracker.md" + +# ======================================== +# Test Group 5: Other modifiers (dd, truncate, exec) +# ======================================== +echo "" +echo "Test Group 5: Other modifiers" +echo "" + +assert_modifies "dd if=/dev/zero of=goal-tracker.md" +assert_modifies "truncate -s 0 goal-tracker.md" +assert_modifies "exec 3> goal-tracker.md" +assert_modifies "printf '%s' data > goal-tracker.md" + +# ======================================== +# Test Group 6: Commands that should NOT be caught +# ======================================== +echo "" +echo "Test Group 6: Commands that should NOT be caught (false positives)" +echo "" + +assert_not_modifies "cat goal-tracker.md" +assert_not_modifies "grep goal goal-tracker.md" +assert_not_modifies "head -10 goal-tracker.md" +assert_not_modifies "tail -10 goal-tracker.md" +assert_not_modifies "wc -l goal-tracker.md" +assert_not_modifies "less goal-tracker.md" +assert_not_modifies "echo goal-tracker.md" +assert_not_modifies "ls goal-tracker.md" +assert_not_modifies "file goal-tracker.md" +assert_not_modifies "stat goal-tracker.md" +assert_not_modifies "diff goal-tracker.md other.md" + +# ======================================== +# Test Group 7: Edge cases +# ======================================== +echo "" +echo "Test Group 7: Edge cases" +echo "" + +# Filename in different positions +assert_modifies "> goal-tracker.md" +assert_modifies "echo test >goal-tracker.md" + +# Multiple source files to single destination +# Note: "cp file1.md file2.md goal-tracker.md" (multiple sources) is NOT detected +# because the pattern expects "cp src dest" format. This is a known limitation. +# The more common "cp src goal-tracker.md" case IS detected. + +# With variables (should still match the literal pattern) +assert_not_modifies 'echo x > $FILE' +assert_not_modifies "cat file.md | grep pattern" + +# ======================================== +# Test Group 8: State file patterns +# ======================================== +echo "" +echo "Test Group 8: State file patterns" +echo "" + +assert_modifies "echo x > state.md" "state\\.md" +assert_modifies "sed -i 's/round: 0/round: 99/' state.md" "state\\.md" +assert_not_modifies "cat state.md" "state\\.md" + +# ======================================== +# Test Group 9: Summary file patterns +# ======================================== +echo "" +echo "Test Group 9: Summary file patterns" +echo "" + +assert_modifies "echo x > round-5-summary.md" "round-[0-9]+-summary\\.md" +assert_modifies "cat data >> round-10-summary.md" "round-[0-9]+-summary\\.md" +assert_not_modifies "cat round-5-summary.md" "round-[0-9]+-summary\\.md" + +# ======================================== +# Summary +# ======================================== +echo "" +echo "========================================" +echo "Test Summary" +echo "========================================" +echo -e "Passed: ${GREEN}$TESTS_PASSED${NC}" +echo -e "Failed: ${RED}$TESTS_FAILED${NC}" + +if [[ $TESTS_FAILED -eq 0 ]]; then + echo "" + echo -e "${GREEN}All tests passed!${NC}" + exit 0 +else + echo "" + echo -e "${RED}Some tests failed!${NC}" + exit 1 +fi diff --git a/tests/test-plan-file-validation.sh b/tests/test-plan-file-validation.sh index 686a2b0b..b6301bf4 100755 --- a/tests/test-plan-file-validation.sh +++ b/tests/test-plan-file-validation.sh @@ -507,6 +507,85 @@ else pass "Branch with quotes rejected (by git)" fi +echo "" +echo "=== Test: Plan File Content Validation ===" +echo "" + +# Test 9.8: Reject plan file with only blank lines +echo "Test 9.8: Reject plan with only blank lines" +cd "$TEST_DIR" +rm -rf content-test 2>/dev/null || true +mkdir -p content-test +cd content-test +git init -q +git config user.email "test@test.com" +git config user.name "Test" +echo "init" > init.txt +git add init.txt +git commit -q -m "Initial" +mkdir -p plans +# Create plan with only blank lines (6 lines total to pass the 5-line minimum) +printf '\n\n\n\n\n\n' > plans/blank-plan.md +echo "plans/" >> .gitignore +git add .gitignore +git commit -q -m "Gitignore" +set +e +RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/blank-plan.md" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -ne 0 ]] && echo "$RESULT" | grep -q "insufficient content"; then + pass "Plan with only blank lines rejected" +else + fail "Blank plan rejection" "exit 1 with insufficient content error" "$RESULT" +fi + +# Test 9.9: Reject plan file with only few non-blank lines +echo "Test 9.9: Reject plan with too few non-blank lines" +# Create plan with mostly blank lines and only 2 non-blank lines +cat > plans/sparse-plan.md << 'EOF' +# Title + + +Only one more line + + +EOF +set +e +RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/sparse-plan.md" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -ne 0 ]] && echo "$RESULT" | grep -q "insufficient content"; then + pass "Plan with too few non-blank lines rejected" +else + fail "Sparse plan rejection" "exit 1 with insufficient content error" "$RESULT" +fi + +# Test 9.10: Accept plan with enough non-blank content +echo "Test 9.10: Accept plan with sufficient non-blank content" +cat > plans/good-plan.md << 'EOF' +# Good Plan + +## Goal +This is a valid plan file with enough content. + +## Requirements +- Requirement 1 +- Requirement 2 + +## Implementation +Details here. +EOF +set +e +RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/good-plan.md" 2>&1) +EXIT_CODE=$? +set -e +# Should not fail due to content validation (may fail later for other reasons like codex) +if ! echo "$RESULT" | grep -q "insufficient content"; then + pass "Valid plan with sufficient content accepted" +else + fail "Valid plan acceptance" "no insufficient content error" "$RESULT" +fi + echo "" echo "=== Test: CLI Options ===" echo "" diff --git a/tests/test-template-loader.sh b/tests/test-template-loader.sh index 85dcc414..bed744b5 100755 --- a/tests/test-template-loader.sh +++ b/tests/test-template-loader.sh @@ -542,6 +542,76 @@ else fail "Realistic injection scenario" "$EXPECTED" "$RESULT" fi +# ======================================== +# Test 37-41: Additional Edge Cases +# ======================================== +# These tests cover additional edge cases for template rendering. + +echo "" +echo "========================================" +echo "Additional Edge Case Tests" +echo "========================================" + +# Test 37: Empty variable substitution +echo "" +echo "Test 37: Empty variable substitution" +TEMPLATE="Hello {{NAME}}, status: {{STATUS}}" +RESULT=$(render_template "$TEMPLATE" "NAME=" "STATUS=active") +EXPECTED="Hello , status: active" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Empty variable substitution works" +else + fail "Empty variable substitution" "$EXPECTED" "$RESULT" +fi + +# Test 38: Unicode characters in template +echo "" +echo "Test 38: Unicode characters in template" +TEMPLATE="Greeting: {{GREETING}}" +RESULT=$(render_template "$TEMPLATE" "GREETING=Hello World") +EXPECTED="Greeting: Hello World" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Unicode in template renders correctly" +else + fail "Unicode in template" "$EXPECTED" "$RESULT" +fi + +# Test 39: Unicode characters in value +echo "" +echo "Test 39: Unicode characters in value" +TEMPLATE="Message: {{MSG}}" +RESULT=$(render_template "$TEMPLATE" "MSG=Bon jour mon ami") +EXPECTED="Message: Bon jour mon ami" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Unicode in value renders correctly" +else + fail "Unicode in value" "$EXPECTED" "$RESULT" +fi + +# Test 40: Variable name edge cases - underscore prefix +echo "" +echo "Test 40: Variable with underscore prefix" +TEMPLATE="Value: {{_PRIVATE}}" +RESULT=$(render_template "$TEMPLATE" "_PRIVATE=secret") +EXPECTED="Value: secret" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Underscore-prefixed variable works" +else + fail "Underscore-prefixed variable" "$EXPECTED" "$RESULT" +fi + +# Test 41: Variable name with numbers +echo "" +echo "Test 41: Variable name with numbers" +TEMPLATE="Round: {{ROUND_1}} and {{ROUND_2}}" +RESULT=$(render_template "$TEMPLATE" "ROUND_1=first" "ROUND_2=second") +EXPECTED="Round: first and second" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Variable names with numbers work" +else + fail "Variable names with numbers" "$EXPECTED" "$RESULT" +fi + # ======================================== # Summary # ======================================== diff --git a/tests/test-todo-checker.sh b/tests/test-todo-checker.sh new file mode 100755 index 00000000..031b121a --- /dev/null +++ b/tests/test-todo-checker.sh @@ -0,0 +1,313 @@ +#!/bin/bash +# +# Test script for check-todos-from-transcript.py +# +# Tests the Python todo checker for proper error handling +# and correct interpretation of todo states. +# + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +TODO_CHECKER="$PROJECT_ROOT/hooks/check-todos-from-transcript.py" + +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' # No Color + +TESTS_PASSED=0 +TESTS_FAILED=0 + +# Test helper functions +pass() { + echo -e "${GREEN}PASS${NC}: $1" + TESTS_PASSED=$((TESTS_PASSED + 1)) +} + +fail() { + echo -e "${RED}FAIL${NC}: $1" + echo " Expected: $2" + echo " Got: $3" + TESTS_FAILED=$((TESTS_FAILED + 1)) +} + +# Setup test environment +TEST_DIR=$(mktemp -d) +trap "rm -rf $TEST_DIR" EXIT + +echo "========================================" +echo "Testing check-todos-from-transcript.py" +echo "========================================" +echo "" + +# ======================================== +# Test Group 1: Input Handling +# ======================================== +echo "Test Group 1: Input Handling" +echo "" + +# Test 1: Invalid JSON input should exit 0 (graceful handling) +echo "Test 1: Invalid JSON input" +set +e +RESULT=$(echo "not json at all" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Invalid JSON handled gracefully (exit 0)" +else + fail "Invalid JSON handling" "exit 0" "exit $EXIT_CODE" +fi + +# Test 2: Empty input should exit 0 +echo "Test 2: Empty input" +set +e +RESULT=$(echo "" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Empty input handled gracefully (exit 0)" +else + fail "Empty input handling" "exit 0" "exit $EXIT_CODE" +fi + +# Test 3: Valid JSON without transcript_path should exit 0 +echo "Test 3: JSON without transcript_path" +set +e +RESULT=$(echo '{"other": "data"}' | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "JSON without transcript_path exits 0" +else + fail "Missing transcript_path" "exit 0" "exit $EXIT_CODE" +fi + +# Test 4: Non-existent transcript file should exit 0 +echo "Test 4: Non-existent transcript file" +set +e +RESULT=$(echo '{"transcript_path": "/nonexistent/path/transcript.jsonl"}' | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Non-existent file exits 0" +else + fail "Non-existent file handling" "exit 0" "exit $EXIT_CODE" +fi + +# ======================================== +# Test Group 2: Todo Detection +# ======================================== +echo "" +echo "Test Group 2: Todo Detection" +echo "" + +# Test 5: Transcript with all completed todos +echo "Test 5: All todos completed" +cat > "$TEST_DIR/transcript-all-complete.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task 1", "status": "completed"}, {"content": "Task 2", "status": "completed"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-all-complete.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "All todos completed exits 0" +else + fail "All todos completed" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 6: Transcript with incomplete todos +echo "Test 6: Incomplete todos" +cat > "$TEST_DIR/transcript-incomplete.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task 1", "status": "completed"}, {"content": "Task 2", "status": "pending"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-incomplete.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 1 ]]; then + pass "Incomplete todos exits 1" +else + fail "Incomplete todos" "exit 1" "exit $EXIT_CODE" +fi + +# Test 7: Output includes incomplete todo details +echo "Test 7: Output includes todo details" +if echo "$RESULT" | grep -q "Task 2"; then + pass "Output includes incomplete task name" +else + fail "Output includes task name" "Task 2 in output" "$RESULT" +fi + +# Test 8: In-progress status counts as incomplete +echo "Test 8: In-progress status" +cat > "$TEST_DIR/transcript-in-progress.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task 1", "status": "in_progress"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-in-progress.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 1 ]]; then + pass "In-progress status exits 1" +else + fail "In-progress status" "exit 1" "exit $EXIT_CODE" +fi + +# ======================================== +# Test Group 3: Transcript Format Variations +# ======================================== +echo "" +echo "Test Group 3: Transcript Format Variations" +echo "" + +# Test 9: Empty transcript file +echo "Test 9: Empty transcript file" +touch "$TEST_DIR/transcript-empty.jsonl" +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-empty.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Empty transcript exits 0" +else + fail "Empty transcript" "exit 0" "exit $EXIT_CODE" +fi + +# Test 10: Transcript with invalid JSONL lines +echo "Test 10: Invalid JSONL lines ignored" +cat > "$TEST_DIR/transcript-invalid-lines.jsonl" << 'EOF' +not json +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task", "status": "completed"}]}}]}} +also not json +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-invalid-lines.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Invalid JSONL lines ignored, valid todo found" +else + fail "Invalid JSONL handling" "exit 0 (valid todo found)" "exit $EXIT_CODE" +fi + +# Test 11: Multiple TodoWrite calls - uses latest +echo "Test 11: Multiple TodoWrite calls uses latest" +cat > "$TEST_DIR/transcript-multiple.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Old Task", "status": "pending"}]}}]}} +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "New Task", "status": "completed"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-multiple.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Uses latest TodoWrite (all completed)" +else + fail "Multiple TodoWrite handling" "exit 0 (latest is completed)" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 12: Direct tool_use entry format +echo "Test 12: Direct tool_use entry format" +cat > "$TEST_DIR/transcript-direct.jsonl" << 'EOF' +{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task", "status": "completed"}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-direct.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Direct tool_use format handled" +else + fail "Direct tool_use format" "exit 0" "exit $EXIT_CODE" +fi + +# Test 13: type: message format +echo "Test 13: Alternative message format" +cat > "$TEST_DIR/transcript-message.jsonl" << 'EOF' +{"type": "message", "content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task", "status": "completed"}]}}]} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-message.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Alternative message format handled" +else + fail "Alternative message format" "exit 0" "exit $EXIT_CODE" +fi + +# ======================================== +# Test Group 4: Edge Cases +# ======================================== +echo "" +echo "Test Group 4: Edge Cases" +echo "" + +# Test 14: Todo with missing status field +echo "Test 14: Todo with missing status" +cat > "$TEST_DIR/transcript-no-status.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task without status"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-no-status.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +# Missing status should be treated as incomplete (not "completed") +if [[ $EXIT_CODE -eq 1 ]]; then + pass "Missing status treated as incomplete" +else + fail "Missing status handling" "exit 1 (incomplete)" "exit $EXIT_CODE" +fi + +# Test 15: Todo with empty content +echo "Test 15: Todo with empty content" +cat > "$TEST_DIR/transcript-empty-content.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "", "status": "pending"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-empty-content.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 1 ]]; then + pass "Empty content todo handled (still incomplete)" +else + fail "Empty content handling" "exit 1" "exit $EXIT_CODE" +fi + +# Test 16: Unicode in todo content +echo "Test 16: Unicode in todo content" +cat > "$TEST_DIR/transcript-unicode.jsonl" << 'EOF' +{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "TodoWrite", "input": {"todos": [{"content": "Task with unicode", "status": "completed"}]}}]}} +EOF +set +e +RESULT=$(echo "{\"transcript_path\": \"$TEST_DIR/transcript-unicode.jsonl\"}" | python3 "$TODO_CHECKER" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Unicode content handled" +else + fail "Unicode content" "exit 0" "exit $EXIT_CODE" +fi + +# ======================================== +# Summary +# ======================================== +echo "" +echo "========================================" +echo "Test Summary" +echo "========================================" +echo -e "Passed: ${GREEN}$TESTS_PASSED${NC}" +echo -e "Failed: ${RED}$TESTS_FAILED${NC}" + +if [[ $TESTS_FAILED -eq 0 ]]; then + echo "" + echo -e "${GREEN}All tests passed!${NC}" + exit 0 +else + echo "" + echo -e "${RED}Some tests failed!${NC}" + exit 1 +fi From ea5751ddbe0acfa9abcdc60d7c6eb1dce0d17f05 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 21:16:08 -0800 Subject: [PATCH 02/17] Refactor: Complete Phase 3-4 blockers from Codex review - Wire parse_state_file into all hooks (loop-codex-stop-hook.sh, loop-plan-file-validator.sh, loop-bash-validator.sh) - Replace hardcoded magic strings with constants (MARKER_COMPLETE, MARKER_STOP, EXIT_* constants) - Add run_with_timeout for git operations in stop hook - Implement exit code 2 on parse errors in todo checker - Fix plan content validation to exclude comment-only plans - Update tests to reflect new exit code behavior - Add test for HTML-comment-only plan files All tests pass: template-loader (41), bash-validator-patterns (48), todo-checker (16), plan-file-validation (26), template-references (74), state-exit-naming (13) --- hooks/check-todos-from-transcript.py | 14 ++++-- hooks/loop-bash-validator.sh | 4 +- hooks/loop-codex-stop-hook.sh | 74 +++++++++++++--------------- hooks/loop-plan-file-validator.sh | 15 +++--- scripts/setup-rlcr-loop.sh | 40 ++++++++++++--- tests/test-plan-file-validation.sh | 23 +++++++++ tests/test-todo-checker.sh | 16 +++--- 7 files changed, 118 insertions(+), 68 deletions(-) diff --git a/hooks/check-todos-from-transcript.py b/hooks/check-todos-from-transcript.py index eb27da93..ab359fcd 100755 --- a/hooks/check-todos-from-transcript.py +++ b/hooks/check-todos-from-transcript.py @@ -3,8 +3,11 @@ Helper script to check for incomplete todos from Claude Code transcript. Reads the transcript JSONL file and finds the most recent TodoWrite tool call. -Returns exit code 0 if all todos are completed (or no todos exist). -Returns exit code 1 if there are incomplete todos, with details on stderr. + +Exit codes: + 0 - All todos are completed (or no todos exist) + 1 - There are incomplete todos (details on stdout) + 2 - Parse error reading hook input JSON Usage: echo '{"transcript_path": "/path/to/transcript.jsonl"}' | python3 check-todos-from-transcript.py @@ -88,9 +91,10 @@ def main(): # Read hook input from stdin try: hook_input = json.load(sys.stdin) - except json.JSONDecodeError: - # No valid input, assume no todos - sys.exit(0) + except json.JSONDecodeError as e: + # Parse error - exit with code 2 + print(f"PARSE_ERROR: {e}", file=sys.stderr) + sys.exit(2) transcript_path = hook_input.get("transcript_path", "") if not transcript_path: diff --git a/hooks/loop-bash-validator.sh b/hooks/loop-bash-validator.sh index 8b198668..f6265e7a 100755 --- a/hooks/loop-bash-validator.sh +++ b/hooks/loop-bash-validator.sh @@ -50,7 +50,9 @@ STATE_FILE="$ACTIVE_LOOP_DIR/state.md" # ======================================== # Default behavior: commits stay local, no need to push to remote -PUSH_EVERY_ROUND=$(grep -E "^push_every_round:" "$STATE_FILE" 2>/dev/null | sed 's/push_every_round: *//' || echo "false") +# Parse state file using shared function +parse_state_file "$STATE_FILE" +PUSH_EVERY_ROUND="$STATE_PUSH_EVERY_ROUND" if [[ "$PUSH_EVERY_ROUND" != "true" ]]; then # Check if command is a git push command diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 420164be..30fa3b5b 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -33,7 +33,7 @@ HOOK_INPUT=$(cat) # from a previous blocked stop. We WANT to run Codex review each iteration. # Loop termination is controlled by: # - No active loop directory (no state.md) -> exit early below -# - Codex outputs "COMPLETE" -> allow exit +# - Codex outputs MARKER_COMPLETE -> allow exit # - current_round >= max_iterations -> allow exit # ======================================== @@ -47,6 +47,13 @@ LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)" source "$SCRIPT_DIR/lib/loop-common.sh" +# Source portable timeout wrapper for git operations +PLUGIN_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +source "$PLUGIN_ROOT/scripts/portable-timeout.sh" + +# Default timeout for git operations (30 seconds) +GIT_TIMEOUT=30 + # Template directory is set by loop-common.sh via template-loader.sh LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") @@ -59,57 +66,44 @@ fi STATE_FILE="$LOOP_DIR/state.md" # ======================================== -# Parse State File (all frontmatter fields) +# Parse State File (using shared function) # ======================================== if [[ ! -f "$STATE_FILE" ]]; then exit 0 fi -FRONTMATTER=$(sed -n '/^---$/,/^---$/{ /^---$/d; p; }' "$STATE_FILE" 2>/dev/null || echo "") - -# Fields for integrity checks (may be empty for old state files) -# Note: Values are unquoted since v1.1.2+ validates paths don't contain special chars -# Legacy quote-stripping kept for backward compatibility with older state files -PLAN_TRACKED=$(echo "$FRONTMATTER" | grep '^plan_tracked:' | sed 's/plan_tracked: *//' | tr -d ' ' || true) -START_BRANCH=$(echo "$FRONTMATTER" | grep '^start_branch:' | sed 's/start_branch: *//; s/^"//; s/"$//' || true) -PLAN_FILE=$(echo "$FRONTMATTER" | grep '^plan_file:' | sed 's/plan_file: *//; s/^"//; s/"$//' || true) - -# Fields for loop iteration control -CURRENT_ROUND=$(echo "$FRONTMATTER" | grep '^current_round:' | sed 's/current_round: *//' | tr -d ' ' || true) -MAX_ITERATIONS=$(echo "$FRONTMATTER" | grep '^max_iterations:' | sed 's/max_iterations: *//' | tr -d ' ' || true) -PUSH_EVERY_ROUND=$(echo "$FRONTMATTER" | grep '^push_every_round:' | sed 's/push_every_round: *//' | tr -d ' ' || true) - -# Fields for Codex configuration -CODEX_MODEL=$(echo "$FRONTMATTER" | grep '^codex_model:' | sed 's/codex_model: *//' | tr -d ' ' || true) -CODEX_EFFORT=$(echo "$FRONTMATTER" | grep '^codex_effort:' | sed 's/codex_effort: *//' | tr -d ' ' || true) -STATE_CODEX_TIMEOUT=$(echo "$FRONTMATTER" | grep '^codex_timeout:' | sed 's/codex_timeout: *//' | tr -d ' ' || true) +# Use shared parsing function from loop-common.sh +parse_state_file "$STATE_FILE" -# Apply defaults -CURRENT_ROUND="${CURRENT_ROUND:-0}" -MAX_ITERATIONS="${MAX_ITERATIONS:-10}" -PUSH_EVERY_ROUND="${PUSH_EVERY_ROUND:-false}" -CODEX_MODEL="${CODEX_MODEL:-$DEFAULT_CODEX_MODEL}" -CODEX_EFFORT="${CODEX_EFFORT:-$DEFAULT_CODEX_EFFORT}" +# Map STATE_* variables to local names for backward compatibility +PLAN_TRACKED="$STATE_PLAN_TRACKED" +START_BRANCH="$STATE_START_BRANCH" +PLAN_FILE="$STATE_PLAN_FILE" +CURRENT_ROUND="$STATE_CURRENT_ROUND" +MAX_ITERATIONS="$STATE_MAX_ITERATIONS" +PUSH_EVERY_ROUND="$STATE_PUSH_EVERY_ROUND" +CODEX_MODEL="${STATE_CODEX_MODEL:-$DEFAULT_CODEX_MODEL}" +CODEX_EFFORT="${STATE_CODEX_EFFORT:-$DEFAULT_CODEX_EFFORT}" CODEX_TIMEOUT="${STATE_CODEX_TIMEOUT:-${CODEX_TIMEOUT:-$DEFAULT_CODEX_TIMEOUT}}" # Re-validate Codex Model and Effort for YAML safety (in case state.md was manually edited) # Use same validation patterns as setup-rlcr-loop.sh if [[ ! "$CODEX_MODEL" =~ ^[a-zA-Z0-9._-]+$ ]]; then echo "Error: Invalid codex_model in state file: $CODEX_MODEL" >&2 - end_loop "$LOOP_DIR" "$STATE_FILE" "unexpected" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_UNEXPECTED" exit 0 fi if [[ ! "$CODEX_EFFORT" =~ ^[a-zA-Z0-9_-]+$ ]]; then echo "Error: Invalid codex_effort in state file: $CODEX_EFFORT" >&2 - end_loop "$LOOP_DIR" "$STATE_FILE" "unexpected" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_UNEXPECTED" exit 0 fi # Validate numeric fields early if [[ ! "$CURRENT_ROUND" =~ ^[0-9]+$ ]]; then echo "Warning: State file corrupted (current_round), stopping loop" >&2 - end_loop "$LOOP_DIR" "$STATE_FILE" "unexpected" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_UNEXPECTED" exit 0 fi @@ -140,7 +134,7 @@ fi # Quick-check 0.5: Branch Consistency # ======================================== -CURRENT_BRANCH=$(git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo "") +CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo "") if [[ -n "$START_BRANCH" && "$CURRENT_BRANCH" != "$START_BRANCH" ]]; then REASON="Git branch changed during RLCR loop. @@ -192,7 +186,7 @@ fi # For gitignored files: check content diff only if [[ "$PLAN_TRACKED" == "true" ]]; then # Tracked file: first check git status for uncommitted changes - PLAN_GIT_STATUS=$(git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") + PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") if [[ -n "$PLAN_GIT_STATUS" ]]; then REASON="Plan file has uncommitted modifications. @@ -275,9 +269,9 @@ fi GIT_STATUS_CACHED="" GIT_IS_REPO=false -if command -v git &>/dev/null && git rev-parse --git-dir &>/dev/null 2>&1; then +if command -v git &>/dev/null && run_with_timeout "$GIT_TIMEOUT" git rev-parse --git-dir &>/dev/null 2>&1; then GIT_IS_REPO=true - GIT_STATUS_CACHED=$(git status --porcelain 2>/dev/null || echo "") + GIT_STATUS_CACHED=$(run_with_timeout "$GIT_TIMEOUT" git status --porcelain 2>/dev/null || echo "") fi # ======================================== @@ -431,10 +425,10 @@ Please commit all changes before exiting. if [[ "$PUSH_EVERY_ROUND" == "true" ]]; then # Check if local branch is ahead of remote (unpushed commits) - GIT_AHEAD=$(git status -sb 2>/dev/null | grep -o 'ahead [0-9]*' || true) + GIT_AHEAD=$(run_with_timeout "$GIT_TIMEOUT" git status -sb 2>/dev/null | grep -o 'ahead [0-9]*' || true) if [[ -n "$GIT_AHEAD" ]]; then AHEAD_COUNT=$(echo "$GIT_AHEAD" | grep -o '[0-9]*') - CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "unknown") + CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "unknown") FALLBACK="# Unpushed Commits @@ -557,7 +551,7 @@ NEXT_ROUND=$((CURRENT_ROUND + 1)) if [[ $NEXT_ROUND -gt $MAX_ITERATIONS ]]; then echo "RLCR loop did not complete, but reached max iterations ($MAX_ITERATIONS). Exiting." >&2 - end_loop "$LOOP_DIR" "$STATE_FILE" "maxiter" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_MAXITER" exit 0 fi @@ -864,18 +858,18 @@ LAST_LINE=$(echo "$REVIEW_CONTENT" | grep -v '^[[:space:]]*$' | tail -1) LAST_LINE_TRIMMED=$(echo "$LAST_LINE" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') # Handle COMPLETE - loop finished successfully -if [[ "$LAST_LINE_TRIMMED" == "COMPLETE" ]]; then +if [[ "$LAST_LINE_TRIMMED" == "$MARKER_COMPLETE" ]]; then if [[ "$FULL_ALIGNMENT_CHECK" == "true" ]]; then echo "Codex review passed. All goals achieved. Loop complete!" >&2 else echo "Codex review passed. Loop complete!" >&2 fi - end_loop "$LOOP_DIR" "$STATE_FILE" "complete" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_COMPLETE" exit 0 fi # Handle STOP - circuit breaker triggered -if [[ "$LAST_LINE_TRIMMED" == "STOP" ]]; then +if [[ "$LAST_LINE_TRIMMED" == "$MARKER_STOP" ]]; then echo "" >&2 echo "========================================" >&2 if [[ "$FULL_ALIGNMENT_CHECK" == "true" ]]; then @@ -900,7 +894,7 @@ if [[ "$LAST_LINE_TRIMMED" == "STOP" ]]; then echo " $REVIEW_RESULT_FILE" >&2 fi echo "========================================" >&2 - end_loop "$LOOP_DIR" "$STATE_FILE" "stop" + end_loop "$LOOP_DIR" "$STATE_FILE" "$EXIT_STOP" exit 0 fi diff --git a/hooks/loop-plan-file-validator.sh b/hooks/loop-plan-file-validator.sh index e628b3f8..dd920825 100755 --- a/hooks/loop-plan-file-validator.sh +++ b/hooks/loop-plan-file-validator.sh @@ -30,14 +30,13 @@ fi STATE_FILE="$LOOP_DIR/state.md" -# Parse state file -# Note: Values are unquoted since v1.1.2+ validates paths don't contain special chars -# Legacy quote-stripping kept for backward compatibility with older state files -FRONTMATTER=$(sed -n '/^---$/,/^---$/{ /^---$/d; p; }' "$STATE_FILE" 2>/dev/null || echo "") - -PLAN_TRACKED=$(echo "$FRONTMATTER" | grep '^plan_tracked:' | sed 's/plan_tracked: *//' | tr -d ' ' || true) -PLAN_FILE=$(echo "$FRONTMATTER" | grep '^plan_file:' | sed 's/plan_file: *//; s/^"//; s/"$//' || true) -START_BRANCH=$(echo "$FRONTMATTER" | grep '^start_branch:' | sed 's/start_branch: *//; s/^"//; s/"$//' || true) +# Parse state file using shared function +parse_state_file "$STATE_FILE" + +# Map STATE_* variables to local names for backward compatibility +PLAN_TRACKED="$STATE_PLAN_TRACKED" +PLAN_FILE="$STATE_PLAN_FILE" +START_BRANCH="$STATE_START_BRANCH" # ======================================== # Schema Validation (v1.1.2+ required fields) diff --git a/scripts/setup-rlcr-loop.sh b/scripts/setup-rlcr-loop.sh index 68ff2061..a9f0b4d9 100755 --- a/scripts/setup-rlcr-loop.sh +++ b/scripts/setup-rlcr-loop.sh @@ -345,13 +345,41 @@ if [[ "$LINE_COUNT" -lt 5 ]]; then exit 1 fi -# Check plan has actual content (not just whitespace/blank lines) -# Exclude blank lines and lines that are only markdown comments () -NON_BLANK_LINES=$(grep -cvE '^[[:space:]]*$' "$FULL_PLAN_PATH" 2>/dev/null || echo "0") -if [[ "$NON_BLANK_LINES" -lt 3 ]]; then - echo "Error: Plan file has insufficient content (only $NON_BLANK_LINES non-blank lines)" >&2 +# Check plan has actual content (not just whitespace/blank lines/comments) +# Exclude: blank lines and HTML comments () +# Note: In markdown, # starts a heading (content), not a comment +# A "content line" is any line that is not blank and not an HTML comment +# For multi-line HTML comments, we count lines inside them as non-content +CONTENT_LINES=0 +IN_COMMENT=false +while IFS= read -r line || [[ -n "$line" ]]; do + # Check for multi-line comment start + if [[ "$line" =~ ^[[:space:]]*\ + + + + + +EOF +set +e +RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/comment-plan.md" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -ne 0 ]] && echo "$RESULT" | grep -q "insufficient content"; then + pass "Plan with only HTML comments rejected" +else + fail "HTML-comment-only plan rejection" "exit 1 with insufficient content error" "$RESULT" +fi + # Test 9.10: Accept plan with enough non-blank content echo "Test 9.10: Accept plan with sufficient non-blank content" cat > plans/good-plan.md << 'EOF' diff --git a/tests/test-todo-checker.sh b/tests/test-todo-checker.sh index 031b121a..a4286020 100755 --- a/tests/test-todo-checker.sh +++ b/tests/test-todo-checker.sh @@ -48,28 +48,28 @@ echo "" echo "Test Group 1: Input Handling" echo "" -# Test 1: Invalid JSON input should exit 0 (graceful handling) +# Test 1: Invalid JSON input should exit 2 (parse error) echo "Test 1: Invalid JSON input" set +e RESULT=$(echo "not json at all" | python3 "$TODO_CHECKER" 2>&1) EXIT_CODE=$? set -e -if [[ $EXIT_CODE -eq 0 ]]; then - pass "Invalid JSON handled gracefully (exit 0)" +if [[ $EXIT_CODE -eq 2 ]]; then + pass "Invalid JSON returns exit code 2" else - fail "Invalid JSON handling" "exit 0" "exit $EXIT_CODE" + fail "Invalid JSON handling" "exit 2" "exit $EXIT_CODE" fi -# Test 2: Empty input should exit 0 +# Test 2: Empty input should exit 2 (parse error) echo "Test 2: Empty input" set +e RESULT=$(echo "" | python3 "$TODO_CHECKER" 2>&1) EXIT_CODE=$? set -e -if [[ $EXIT_CODE -eq 0 ]]; then - pass "Empty input handled gracefully (exit 0)" +if [[ $EXIT_CODE -eq 2 ]]; then + pass "Empty input returns exit code 2" else - fail "Empty input handling" "exit 0" "exit $EXIT_CODE" + fail "Empty input handling" "exit 2" "exit $EXIT_CODE" fi # Test 3: Valid JSON without transcript_path should exit 0 From fed8b43c356535b40fa7597a1b77e25e0fc7d7b8 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 21:36:00 -0800 Subject: [PATCH 03/17] Refactor: Complete all remaining blockers from Round 1 Codex review Phase 3.1/3.2 State parsing standardization: - Replace get_current_round with parse_state_file in read/write/edit validators - Update get_current_round to use FIELD_CURRENT_ROUND constant - Apply FIELD_* constants in loop-plan-file-validator.sh required field list Phase 4.2 Git timeout wrapper completion: - Add run_with_timeout to all git calls in setup-rlcr-loop.sh - Add run_with_timeout to all git calls in loop-plan-file-validator.sh - Make stop hook block on git timeout failure instead of empty fallback Phase 4.3 Todo checker error handling: - Handle exit code 2 in stop hook (block and surface parse error) Phase 1.5/2.3 Plan content validation: - Update validation to treat # lines as non-content (per original plan) - Add #-comment-only plan test (Test 9.9.2) Phase 2.1 Unicode test coverage: - Fix Unicode tests to use actual non-ASCII characters (Tests 38.1, 39.1) AC6 Test runner: - Create tests/run-all-tests.sh for full test suite verification All tests pass: - test-template-loader.sh: 43/43 - test-plan-file-validation.sh: 27/27 - test-todo-checker.sh: 16/16 - test-bash-validator-patterns.sh: 48/48 - test-template-references.sh: 74/74 - test-state-exit-naming.sh: 13/13 --- hooks/lib/loop-common.sh | 3 +- hooks/loop-codex-stop-hook.sh | 35 ++++++++++- hooks/loop-edit-validator.sh | 4 +- hooks/loop-plan-file-validator.sh | 19 ++++-- hooks/loop-read-validator.sh | 4 +- hooks/loop-write-validator.sh | 4 +- scripts/setup-rlcr-loop.sh | 48 ++++++++++----- tests/run-all-tests.sh | 99 ++++++++++++++++++++++++++++++ tests/test-plan-file-validation.sh | 29 +++++++-- tests/test-template-loader.sh | 30 ++++++++- 10 files changed, 242 insertions(+), 33 deletions(-) create mode 100755 tests/run-all-tests.sh diff --git a/hooks/lib/loop-common.sh b/hooks/lib/loop-common.sh index 78434069..1f29cdfd 100755 --- a/hooks/lib/loop-common.sh +++ b/hooks/lib/loop-common.sh @@ -80,6 +80,7 @@ find_active_loop() { # Extract current round number from state.md # Outputs the round number to stdout, defaults to 0 +# Note: For full state parsing, use parse_state_file() instead get_current_round() { local state_file="$1" @@ -87,7 +88,7 @@ get_current_round() { frontmatter=$(sed -n '/^---$/,/^---$/{ /^---$/d; p; }' "$state_file" 2>/dev/null || echo "") local current_round - current_round=$(echo "$frontmatter" | grep '^current_round:' | sed 's/current_round: *//' | tr -d ' ') + current_round=$(echo "$frontmatter" | grep "^${FIELD_CURRENT_ROUND}:" | sed "s/${FIELD_CURRENT_ROUND}: *//" | tr -d ' ') echo "${current_round:-0}" } diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 30fa3b5b..8e7af1c1 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -134,7 +134,21 @@ fi # Quick-check 0.5: Branch Consistency # ======================================== -CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo "") +CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null) +GIT_EXIT_CODE=$? +if [[ $GIT_EXIT_CODE -ne 0 || -z "$CURRENT_BRANCH" ]]; then + REASON="Git operation failed or timed out. + +Cannot verify branch consistency. This may indicate: +- Git is not responding +- Repository is in an invalid state +- Network issues (if remote operations are involved) + +Please check git status manually and try again." + jq -n --arg reason "$REASON" --arg msg "Loop: Blocked - git operation failed" \ + '{"decision": "block", "reason": $reason, "systemMessage": $msg}' + exit 0 +fi if [[ -n "$START_BRANCH" && "$CURRENT_BRANCH" != "$START_BRANCH" ]]; then REASON="Git branch changed during RLCR loop. @@ -235,6 +249,25 @@ if [[ -f "$TODO_CHECKER" ]]; then TODO_RESULT=$(echo "$HOOK_INPUT" | python3 "$TODO_CHECKER" 2>&1) || TODO_EXIT=$? TODO_EXIT=${TODO_EXIT:-0} + if [[ "$TODO_EXIT" -eq 2 ]]; then + # Parse error - block and surface the error + REASON="Todo checker encountered a parse error. + +Error: $TODO_RESULT + +This may indicate an issue with the hook input or transcript format. +Please try again or cancel the loop if this persists." + jq -n \ + --arg reason "$REASON" \ + --arg msg "Loop: Blocked - todo checker parse error" \ + '{ + "decision": "block", + "reason": $reason, + "systemMessage": $msg + }' + exit 0 + fi + if [[ "$TODO_EXIT" -eq 1 ]]; then # Incomplete todos found - block immediately without Codex review # Extract the incomplete todo list from the result diff --git a/hooks/loop-edit-validator.sh b/hooks/loop-edit-validator.sh index f611d6fa..01aea848 100755 --- a/hooks/loop-edit-validator.sh +++ b/hooks/loop-edit-validator.sh @@ -63,7 +63,9 @@ if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 fi -CURRENT_ROUND=$(get_current_round "$ACTIVE_LOOP_DIR/state.md") +# Parse state file using shared function +parse_state_file "$ACTIVE_LOOP_DIR/state.md" +CURRENT_ROUND="$STATE_CURRENT_ROUND" # ======================================== # Block State File Edits diff --git a/hooks/loop-plan-file-validator.sh b/hooks/loop-plan-file-validator.sh index dd920825..190dcbf6 100755 --- a/hooks/loop-plan-file-validator.sh +++ b/hooks/loop-plan-file-validator.sh @@ -16,6 +16,13 @@ PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" # Source shared loop functions and template loader source "$SCRIPT_DIR/lib/loop-common.sh" +# Source portable timeout wrapper for git operations +PLUGIN_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +source "$PLUGIN_ROOT/scripts/portable-timeout.sh" + +# Default timeout for git operations (30 seconds) +GIT_TIMEOUT=30 + # Read hook input (required for UserPromptSubmit hooks) INPUT=$(cat) @@ -62,8 +69,8 @@ schema_validation_error() { EOF } -# Check required fields -REQUIRED_FIELDS=("plan_tracked:$PLAN_TRACKED" "start_branch:$START_BRANCH") +# Check required fields (using FIELD_* constants from loop-common.sh) +REQUIRED_FIELDS=("${FIELD_PLAN_TRACKED}:$PLAN_TRACKED" "${FIELD_START_BRANCH}:$START_BRANCH") for field_entry in "${REQUIRED_FIELDS[@]}"; do field_name="${field_entry%%:*}" field_value="${field_entry#*:}" @@ -78,7 +85,7 @@ done # Branch Consistency Check # ======================================== -CURRENT_BRANCH=$(git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo "") +CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo "") if [[ -n "$START_BRANCH" && "$CURRENT_BRANCH" != "$START_BRANCH" ]]; then cat << EOF { @@ -97,8 +104,8 @@ FULL_PLAN_PATH="$PROJECT_ROOT/$PLAN_FILE" if [[ "$PLAN_TRACKED" == "true" ]]; then # Must be tracked and clean - PLAN_IS_TRACKED=$(git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") - PLAN_GIT_STATUS=$(git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") + PLAN_IS_TRACKED=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") + PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") if [[ "$PLAN_IS_TRACKED" != "true" ]]; then cat << EOF @@ -121,7 +128,7 @@ EOF fi else # Must be gitignored (not tracked) - PLAN_IS_TRACKED=$(git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") + PLAN_IS_TRACKED=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") if [[ "$PLAN_IS_TRACKED" == "true" ]]; then cat << EOF diff --git a/hooks/loop-read-validator.sh b/hooks/loop-read-validator.sh index 4ddbb885..945f2ea0 100755 --- a/hooks/loop-read-validator.sh +++ b/hooks/loop-read-validator.sh @@ -61,7 +61,9 @@ if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 fi -CURRENT_ROUND=$(get_current_round "$ACTIVE_LOOP_DIR/state.md") +# Parse state file using shared function +parse_state_file "$ACTIVE_LOOP_DIR/state.md" +CURRENT_ROUND="$STATE_CURRENT_ROUND" # ======================================== # Extract Round Number and File Type diff --git a/hooks/loop-write-validator.sh b/hooks/loop-write-validator.sh index 719f2464..88543f5c 100755 --- a/hooks/loop-write-validator.sh +++ b/hooks/loop-write-validator.sh @@ -78,7 +78,9 @@ if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 fi -CURRENT_ROUND=$(get_current_round "$ACTIVE_LOOP_DIR/state.md") +# Parse state file using shared function +parse_state_file "$ACTIVE_LOOP_DIR/state.md" +CURRENT_ROUND="$STATE_CURRENT_ROUND" # ======================================== # Block State File Writes diff --git a/scripts/setup-rlcr-loop.sh b/scripts/setup-rlcr-loop.sh index a9f0b4d9..256b8963 100755 --- a/scripts/setup-rlcr-loop.sh +++ b/scripts/setup-rlcr-loop.sh @@ -20,6 +20,13 @@ DEFAULT_CODEX_EFFORT="high" DEFAULT_CODEX_TIMEOUT=5400 DEFAULT_MAX_ITERATIONS=42 +# Default timeout for git operations (30 seconds) +GIT_TIMEOUT=30 + +# Source portable timeout wrapper +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)" +source "$SCRIPT_DIR/portable-timeout.sh" + # ======================================== # Parse Arguments # ======================================== @@ -201,15 +208,15 @@ fi # Git Repository Validation # ======================================== -# Check git repo -if ! git rev-parse --git-dir &>/dev/null; then - echo "Error: Project must be a git repository" >&2 +# Check git repo (with timeout) +if ! run_with_timeout "$GIT_TIMEOUT" git rev-parse --git-dir &>/dev/null; then + echo "Error: Project must be a git repository (or git command timed out)" >&2 exit 1 fi -# Check at least one commit -if ! git rev-parse HEAD &>/dev/null 2>&1; then - echo "Error: Git repository must have at least one commit" >&2 +# Check at least one commit (with timeout) +if ! run_with_timeout "$GIT_TIMEOUT" git rev-parse HEAD &>/dev/null 2>&1; then + echo "Error: Git repository must have at least one commit (or git command timed out)" >&2 exit 1 fi @@ -285,9 +292,9 @@ fi # Check not in submodule # Quick check: only run expensive git submodule status if .gitmodules exists if [[ -f "$PROJECT_ROOT/.gitmodules" ]]; then - if git -C "$PROJECT_ROOT" submodule status 2>/dev/null | grep -q .; then + if run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" submodule status 2>/dev/null | grep -q .; then # Get list of submodule paths - SUBMODULES=$(git -C "$PROJECT_ROOT" submodule status | awk '{print $2}') + SUBMODULES=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" submodule status | awk '{print $2}') for submod in $SUBMODULES; do if [[ "$PLAN_FILE" = "$submod"/* || "$PLAN_FILE" = "$submod" ]]; then echo "Error: Plan file cannot be inside a git submodule: $submod" >&2 @@ -301,8 +308,8 @@ fi # Plan File Tracking Status Validation # ======================================== -PLAN_GIT_STATUS=$(git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") -PLAN_IS_TRACKED=$(git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") +PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null || echo "") +PLAN_IS_TRACKED=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null && echo "true" || echo "false") if [[ "$TRACK_PLAN_FILE" == "true" ]]; then # Must be tracked and clean @@ -346,14 +353,14 @@ if [[ "$LINE_COUNT" -lt 5 ]]; then fi # Check plan has actual content (not just whitespace/blank lines/comments) -# Exclude: blank lines and HTML comments () -# Note: In markdown, # starts a heading (content), not a comment -# A "content line" is any line that is not blank and not an HTML comment +# Exclude: blank lines, shell/YAML comments (# ...), and HTML comments () +# Note: Lines starting with # are treated as comments, not markdown headings +# A "content line" is any line that is not blank and not purely a comment # For multi-line HTML comments, we count lines inside them as non-content CONTENT_LINES=0 IN_COMMENT=false while IFS= read -r line || [[ -n "$line" ]]; do - # Check for multi-line comment start + # Check for multi-line HTML comment start () if [[ "$line" =~ ^[[:space:]]*\) BEFORE multi-line start detection - tests/test-plan-file-validation.sh: Add Test 9.10.1 regression test for single-line HTML comment + valid content acceptance AC7 verified: Version 1.1.5 in .claude-plugin/plugin.json, .claude-plugin/marketplace.json, and README.md. All 9 test suites pass (28 plan-file-validation, 28 plan-file-hooks, etc.) --- hooks/loop-plan-file-validator.sh | 17 ++++++++++------- scripts/setup-rlcr-loop.sh | 16 +++++++++------- tests/test-plan-file-validation.sh | 26 ++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/hooks/loop-plan-file-validator.sh b/hooks/loop-plan-file-validator.sh index 63a59945..ea001205 100755 --- a/hooks/loop-plan-file-validator.sh +++ b/hooks/loop-plan-file-validator.sh @@ -85,8 +85,9 @@ done # Branch Consistency Check # ======================================== -CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null) -GIT_EXIT_CODE=$? +# Use || GIT_EXIT_CODE=$? to prevent set -e from aborting on non-zero exit +CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null) || GIT_EXIT_CODE=$? +GIT_EXIT_CODE=${GIT_EXIT_CODE:-0} if [[ $GIT_EXIT_CODE -ne 0 || -z "$CURRENT_BRANCH" ]]; then cat << EOF { @@ -114,9 +115,10 @@ FULL_PLAN_PATH="$PROJECT_ROOT/$PLAN_FILE" if [[ "$PLAN_TRACKED" == "true" ]]; then # Must be tracked and clean - # Check if git commands succeed - fail closed on timeout/error - run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null - LS_FILES_EXIT=$? + # Use || LS_FILES_EXIT=$? to prevent set -e from aborting on non-zero exit + # ls-files --error-unmatch returns: 0 (tracked), 1 (not tracked), 124 (timeout) + run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null || LS_FILES_EXIT=$? + LS_FILES_EXIT=${LS_FILES_EXIT:-0} if [[ $LS_FILES_EXIT -eq 124 ]]; then # Timeout - fail closed cat << EOF @@ -129,8 +131,9 @@ EOF fi PLAN_IS_TRACKED=$([[ $LS_FILES_EXIT -eq 0 ]] && echo "true" || echo "false") - PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null) - STATUS_EXIT=$? + # Use || STATUS_EXIT=$? to prevent set -e from aborting on non-zero exit + PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null) || STATUS_EXIT=$? + STATUS_EXIT=${STATUS_EXIT:-0} if [[ $STATUS_EXIT -eq 124 ]]; then # Timeout - fail closed cat << EOF diff --git a/scripts/setup-rlcr-loop.sh b/scripts/setup-rlcr-loop.sh index eeb79803..0299aac2 100755 --- a/scripts/setup-rlcr-loop.sh +++ b/scripts/setup-rlcr-loop.sh @@ -377,12 +377,7 @@ fi CONTENT_LINES=0 IN_COMMENT=false while IFS= read -r line || [[ -n "$line" ]]; do - # Check for multi-line HTML comment start () - if [[ "$line" =~ ^[[:space:]]*\ on same line if [[ "$line" =~ ^[[:space:]]*\ on same line) + # Only trigger if the line contains + if [[ "$line" =~ ^[[:space:]]*\ +This plan has real content + +Goal +The goal is to test single-line comment handling. + +Requirements +- Requirement 1 +- Requirement 2 +- Requirement 3 +EOF +set +e +RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/single-line-html-comment-plan.md" 2>&1) +EXIT_CODE=$? +set -e +# Should not fail due to content validation - single-line comments should be skipped properly +if ! echo "$RESULT" | grep -q "insufficient content"; then + pass "Plan with single-line HTML comments + valid content accepted" +else + fail "Single-line HTML comment handling" "no insufficient content error" "$RESULT" +fi + echo "" echo "=== Test: CLI Options ===" echo "" From 7de16966496e18c572d91492c933f722e33c5f0c Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 22:16:55 -0800 Subject: [PATCH 06/17] Refactor: Complete fail-closed handling and fix test harness signing - hooks/loop-codex-stop-hook.sh: Apply || EXIT=$? pattern to branch check to prevent set -e from aborting before JSON block output - hooks/loop-plan-file-validator.sh: Handle non-timeout git errors (exit codes other than 0, 1, 124) by blocking with explicit error message - tests/test-plan-file-validation.sh: Add -c commit.gpgsign=false to all git commit calls to work in signing-enforced environments - tests/test-plan-file-hooks.sh: Same fix for git commit signing - tests/test-state-exit-naming.sh: Same fix for git commit signing All git operations now handle exit codes: - 0: Success (tracked/clean) - 1: Expected failure (untracked file for ls-files) - 124: Timeout (block with timeout message) - Other: Unexpected error (block with error message) All 9 test suites pass (28 plan-file-validation, 28 plan-file-hooks, etc.) --- hooks/loop-codex-stop-hook.sh | 5 +++-- hooks/loop-plan-file-validator.sh | 35 ++++++++++++++++++++++++++---- tests/test-plan-file-hooks.sh | 14 ++++++------ tests/test-plan-file-validation.sh | 30 ++++++++++++------------- tests/test-state-exit-naming.sh | 2 +- 5 files changed, 57 insertions(+), 29 deletions(-) diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 8e7af1c1..3c50eecd 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -134,8 +134,9 @@ fi # Quick-check 0.5: Branch Consistency # ======================================== -CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null) -GIT_EXIT_CODE=$? +# Use || GIT_EXIT_CODE=$? to prevent set -e from aborting on non-zero exit +CURRENT_BRANCH=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null) || GIT_EXIT_CODE=$? +GIT_EXIT_CODE=${GIT_EXIT_CODE:-0} if [[ $GIT_EXIT_CODE -ne 0 || -z "$CURRENT_BRANCH" ]]; then REASON="Git operation failed or timed out. diff --git a/hooks/loop-plan-file-validator.sh b/hooks/loop-plan-file-validator.sh index ea001205..b197305d 100755 --- a/hooks/loop-plan-file-validator.sh +++ b/hooks/loop-plan-file-validator.sh @@ -116,7 +116,7 @@ FULL_PLAN_PATH="$PROJECT_ROOT/$PLAN_FILE" if [[ "$PLAN_TRACKED" == "true" ]]; then # Must be tracked and clean # Use || LS_FILES_EXIT=$? to prevent set -e from aborting on non-zero exit - # ls-files --error-unmatch returns: 0 (tracked), 1 (not tracked), 124 (timeout) + # ls-files --error-unmatch returns: 0 (tracked), 1 (not tracked), 124 (timeout), other (error) run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null || LS_FILES_EXIT=$? LS_FILES_EXIT=${LS_FILES_EXIT:-0} if [[ $LS_FILES_EXIT -eq 124 ]]; then @@ -126,12 +126,22 @@ if [[ "$PLAN_TRACKED" == "true" ]]; then "decision": "block", "reason": "Git operation timed out while checking plan file tracking status.\\n\\nPlease check git status and try again." } +EOF + exit 0 + elif [[ $LS_FILES_EXIT -ne 0 && $LS_FILES_EXIT -ne 1 ]]; then + # Unexpected git error - fail closed + cat << EOF +{ + "decision": "block", + "reason": "Git operation failed while checking plan file tracking status (exit code: $LS_FILES_EXIT).\\n\\nPlease check git status and try again." +} EOF exit 0 fi PLAN_IS_TRACKED=$([[ $LS_FILES_EXIT -eq 0 ]] && echo "true" || echo "false") # Use || STATUS_EXIT=$? to prevent set -e from aborting on non-zero exit + # git status --porcelain returns: 0 (success), 124 (timeout), other (error) PLAN_GIT_STATUS=$(run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" status --porcelain "$PLAN_FILE" 2>/dev/null) || STATUS_EXIT=$? STATUS_EXIT=${STATUS_EXIT:-0} if [[ $STATUS_EXIT -eq 124 ]]; then @@ -141,6 +151,15 @@ EOF "decision": "block", "reason": "Git operation timed out while checking plan file status.\\n\\nPlease check git status and try again." } +EOF + exit 0 + elif [[ $STATUS_EXIT -ne 0 ]]; then + # Unexpected git error - fail closed + cat << EOF +{ + "decision": "block", + "reason": "Git operation failed while checking plan file status (exit code: $STATUS_EXIT).\\n\\nPlease check git status and try again." +} EOF exit 0 fi @@ -166,9 +185,8 @@ EOF fi else # Must be gitignored (not tracked) - # Check if git command succeeds - fail closed on timeout - # ls-files --error-unmatch returns 1 for untracked files (expected behavior) - # We need to distinguish between: 0 (tracked), 1 (not tracked), 124 (timeout) + # Check if git command succeeds - fail closed on timeout/error + # ls-files --error-unmatch returns: 0 (tracked), 1 (not tracked), 124 (timeout), other (error) run_with_timeout "$GIT_TIMEOUT" git -C "$PROJECT_ROOT" ls-files --error-unmatch "$PLAN_FILE" &>/dev/null || LS_FILES_EXIT=$? LS_FILES_EXIT=${LS_FILES_EXIT:-0} if [[ $LS_FILES_EXIT -eq 124 ]]; then @@ -178,6 +196,15 @@ else "decision": "block", "reason": "Git operation timed out while checking plan file tracking status.\\n\\nPlease check git status and try again." } +EOF + exit 0 + elif [[ $LS_FILES_EXIT -ne 0 && $LS_FILES_EXIT -ne 1 ]]; then + # Unexpected git error - fail closed + cat << EOF +{ + "decision": "block", + "reason": "Git operation failed while checking plan file tracking status (exit code: $LS_FILES_EXIT).\\n\\nPlease check git status and try again." +} EOF exit 0 fi diff --git a/tests/test-plan-file-hooks.sh b/tests/test-plan-file-hooks.sh index 95711d19..08fb76f1 100755 --- a/tests/test-plan-file-hooks.sh +++ b/tests/test-plan-file-hooks.sh @@ -44,7 +44,7 @@ setup_test_loop() { git config user.name "Test" echo "initial" > init.txt git add init.txt - git commit -q -m "Initial commit" + git -c commit.gpgsign=false commit -q -m "Initial commit" # Capture default branch name (main or master depending on git version) DEFAULT_BRANCH=$(git rev-parse --abbrev-ref HEAD) fi @@ -67,7 +67,7 @@ Test the RLCR loop EOF echo "plans/" >> .gitignore git add .gitignore - git commit -q -m "Add gitignore" + git -c commit.gpgsign=false commit -q -m "Add gitignore" # Create plan backup cp plans/test-plan.md "$LOOP_DIR/plan.md" @@ -570,7 +570,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" # Get the default branch name for this new repo TEST12_BRANCH=$(git rev-parse --abbrev-ref HEAD) # Create tracked plan file @@ -582,7 +582,7 @@ Test tracked file - Requirement 1 EOF git add tracked-plan.md -git commit -q -m "Add plan" +git -c commit.gpgsign=false commit -q -m "Add plan" # Create loop directory TRACKED_LOOP_DIR="$PWD/.humanize/rlcr/2024-01-01_12-00-00" mkdir -p "$TRACKED_LOOP_DIR" @@ -664,7 +664,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" # Get the default branch name for this new repo TEST14_BRANCH=$(git rev-parse --abbrev-ref HEAD) # Create tracked plan file @@ -676,7 +676,7 @@ Test tracked file - Requirement 1 EOF git add tracked-plan.md -git commit -q -m "Add plan" +git -c commit.gpgsign=false commit -q -m "Add plan" # Create loop directory and backup TRACKED_LOOP_DIR="$PWD/.humanize/rlcr/2024-01-01_12-00-00" mkdir -p "$TRACKED_LOOP_DIR" @@ -711,7 +711,7 @@ EOF # Modify and COMMIT the plan file (git status will be clean) echo "# Modified and committed" >> tracked-plan.md git add tracked-plan.md -git commit -q -m "Modify plan" +git -c commit.gpgsign=false commit -q -m "Modify plan" # Verify git status is clean for the plan file GIT_STATUS_CHECK=$(git status --porcelain tracked-plan.md) if [[ -n "$GIT_STATUS_CHECK" ]]; then diff --git a/tests/test-plan-file-validation.sh b/tests/test-plan-file-validation.sh index e18829b7..6422e2f1 100755 --- a/tests/test-plan-file-validation.sh +++ b/tests/test-plan-file-validation.sh @@ -43,7 +43,7 @@ setup_test_repo() { git config user.name "Test" echo "initial" > init.txt git add init.txt - git commit -q -m "Initial commit" + git -c commit.gpgsign=false commit -q -m "Initial commit" # Create test plan files mkdir -p plans @@ -62,7 +62,7 @@ EOF # Add plans/ to gitignore (default behavior) echo "plans/" >> .gitignore git add .gitignore - git commit -q -m "Add gitignore" + git -c commit.gpgsign=false commit -q -m "Add gitignore" fi } @@ -205,7 +205,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" # Create a plan directory that we'll make inaccessible mkdir -p plans cat > plans/plan.md << 'EOF' @@ -218,7 +218,7 @@ Test path resolution EOF echo "plans/" >> .gitignore git add .gitignore -git commit -q -m "Gitignore" +git -c commit.gpgsign=false commit -q -m "Gitignore" # Make the plans directory unreadable (if we have permission to do so) if chmod 000 plans 2>/dev/null; then set +e @@ -256,7 +256,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" set +e RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "../outside/escape-plan.md" 2>&1) EXIT_CODE=$? @@ -333,7 +333,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" cat > tracked-plan.md << 'EOF' # Tracked Plan ## Goal @@ -343,7 +343,7 @@ Test tracking - Requirement 2 EOF git add tracked-plan.md -git commit -q -m "Add plan" +git -c commit.gpgsign=false commit -q -m "Add plan" set +e RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "tracked-plan.md" 2>&1) EXIT_CODE=$? @@ -365,7 +365,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" mkdir -p plans cat > plans/untracked-plan.md << 'EOF' # Untracked Plan @@ -377,7 +377,7 @@ Test untracked EOF echo "plans/" >> .gitignore git add .gitignore -git commit -q -m "Gitignore" +git -c commit.gpgsign=false commit -q -m "Gitignore" set +e RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" --track-plan-file "plans/untracked-plan.md" 2>&1) EXIT_CODE=$? @@ -399,7 +399,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" cat > modified-plan.md << 'EOF' # Modified Plan ## Goal @@ -409,7 +409,7 @@ Test modified - Requirement 2 EOF git add modified-plan.md -git commit -q -m "Add plan" +git -c commit.gpgsign=false commit -q -m "Add plan" echo "# Extra line" >> modified-plan.md set +e RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" --track-plan-file "modified-plan.md" 2>&1) @@ -438,7 +438,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" # Get the default branch name for this repo (main or master) BRANCH_TEST_DEFAULT=$(git rev-parse --abbrev-ref HEAD) mkdir -p plans @@ -452,7 +452,7 @@ Test branch validation EOF echo "plans/" >> .gitignore git add .gitignore -git commit -q -m "Gitignore" +git -c commit.gpgsign=false commit -q -m "Gitignore" # Try to create branch with colon (YAML-unsafe) - git may reject this if git checkout -q -b "feature:test" 2>/dev/null; then set +e @@ -522,13 +522,13 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" mkdir -p plans # Create plan with only blank lines (6 lines total to pass the 5-line minimum) printf '\n\n\n\n\n\n' > plans/blank-plan.md echo "plans/" >> .gitignore git add .gitignore -git commit -q -m "Gitignore" +git -c commit.gpgsign=false commit -q -m "Gitignore" set +e RESULT=$("$PROJECT_ROOT/scripts/setup-rlcr-loop.sh" "plans/blank-plan.md" 2>&1) EXIT_CODE=$? diff --git a/tests/test-state-exit-naming.sh b/tests/test-state-exit-naming.sh index 0320efad..4ff3c47a 100755 --- a/tests/test-state-exit-naming.sh +++ b/tests/test-state-exit-naming.sh @@ -43,7 +43,7 @@ git config user.email "test@test.com" git config user.name "Test" echo "init" > init.txt git add init.txt -git commit -q -m "Initial" +git -c commit.gpgsign=false commit -q -m "Initial" LOOP_DIR="$TEST_DIR/.humanize/rlcr/2024-01-01_12-00-00" mkdir -p "$LOOP_DIR" From d00d86b67e6025e7be36f34a38ad4bf2dbd63515 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 22:25:25 -0800 Subject: [PATCH 07/17] Test: Unset CLAUDE_PROJECT_DIR in test-plan-file-validation.sh When CLAUDE_PROJECT_DIR is set (e.g., in an RLCR loop), setup-rlcr-loop.sh uses it instead of pwd. This caused tests to resolve plan file paths against the wrong directory (repo root instead of temp test repo). Fix: Unset CLAUDE_PROJECT_DIR at the start of the test file so the setup script uses pwd (the temp test repo) for each test case. All 9 test suites now pass in all environments (AC6 verified). --- tests/test-plan-file-validation.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test-plan-file-validation.sh b/tests/test-plan-file-validation.sh index 6422e2f1..5317e945 100755 --- a/tests/test-plan-file-validation.sh +++ b/tests/test-plan-file-validation.sh @@ -16,6 +16,10 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +# Unset CLAUDE_PROJECT_DIR so setup-rlcr-loop.sh uses pwd (the temp test repo) +# instead of the actual repo root where this test is running +unset CLAUDE_PROJECT_DIR + # Test helpers GREEN='\033[0;32m' RED='\033[0;31m' From d4e714a5a512fd8d7344a1c5b1f3a9d96042a5b7 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 22:36:59 -0800 Subject: [PATCH 08/17] Fix: Section-specific placeholder detection in stop hook Changed placeholder detection from regex patterns that overlapped across sections to section-specific fixed string patterns using grep -qF. This ensures a placeholder in one section doesn't incorrectly flag other sections. Added 4 tests (14.1-14.4) to verify each placeholder is detected independently and all three are reported when all are present. --- hooks/loop-codex-stop-hook.sh | 13 +- tests/test-plan-file-hooks.sh | 275 ++++++++++++++++++++++++++++++++++ 2 files changed, 283 insertions(+), 5 deletions(-) diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 3c50eecd..20b288df 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -527,17 +527,20 @@ if [[ "$CURRENT_ROUND" -eq 0 ]] && [[ -f "$GOAL_TRACKER_FILE" ]]; then HAS_AC_PLACEHOLDER=false HAS_TASKS_PLACEHOLDER=false - # Use a generic placeholder pattern to detect uninitialized sections - # This matches "[To be extracted/defined/populated ..." patterns more robustly - if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*plan'; then + # Use section-specific placeholder patterns to avoid overlap + # Each pattern matches the unique text for that section only + # Ultimate Goal: "[To be extracted from plan by Claude in Round 0]" + if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be extracted from plan'; then HAS_GOAL_PLACEHOLDER=true fi - if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*Claude.*Round 0'; then + # Acceptance Criteria: "[To be defined by Claude in Round 0 based on the plan]" + if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be defined by Claude'; then HAS_AC_PLACEHOLDER=true fi - if echo "$GOAL_TRACKER_CONTENT" | grep -qE '\[To be (extracted|defined|populated) .*Claude.*plan\]'; then + # Active Tasks: "[To be populated by Claude based on plan]" + if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be populated by Claude'; then HAS_TASKS_PLACEHOLDER=true fi diff --git a/tests/test-plan-file-hooks.sh b/tests/test-plan-file-hooks.sh index 08fb76f1..3f2be029 100755 --- a/tests/test-plan-file-hooks.sh +++ b/tests/test-plan-file-hooks.sh @@ -730,6 +730,281 @@ else fi fi +echo "" +echo "=== Test: Section-Specific Placeholder Detection ===" +echo "" + +# Test 14.1: Stop hook only reports Ultimate Goal placeholder when only that is missing +echo "Test 14.1: Stop hook only reports Ultimate Goal placeholder" +cd "$TEST_DIR" +rm -rf placeholder-test-14-1 2>/dev/null || true +mkdir -p placeholder-test-14-1 +cd placeholder-test-14-1 +git init -q +git config user.email "test@test.com" +git config user.name "Test" +echo "init" > init.txt +# Add .humanize to gitignore so it doesn't trigger uncommitted changes +echo ".humanize*" > .gitignore +git add init.txt .gitignore +git -c commit.gpgsign=false commit -q -m "Initial" +TEST_BRANCH=$(git rev-parse --abbrev-ref HEAD) +# Create gitignored plan +mkdir -p plans +echo "plans/" >> .gitignore +cat > plans/test-plan.md << 'EOF' +# Test Plan +## Goal +Test +EOF +git add .gitignore +git -c commit.gpgsign=false commit -q -m "Add gitignore" +# Create loop directory +LOOP_DIR_14_1="$PWD/.humanize/rlcr/2024-01-01_12-00-00" +mkdir -p "$LOOP_DIR_14_1" +cp plans/test-plan.md "$LOOP_DIR_14_1/plan.md" +cat > "$LOOP_DIR_14_1/state.md" << EOF +--- +current_round: 0 +max_iterations: 42 +plan_file: "plans/test-plan.md" +plan_tracked: false +start_branch: $TEST_BRANCH +--- +EOF +cat > "$LOOP_DIR_14_1/round-0-summary.md" << 'EOF' +# Summary +Work done. +EOF +# Goal tracker with ONLY Ultimate Goal placeholder (AC and Tasks are filled) +cat > "$LOOP_DIR_14_1/goal-tracker.md" << 'EOF' +# Goal Tracker +## IMMUTABLE SECTION +### Ultimate Goal +[To be extracted from plan by Claude in Round 0] +### Acceptance Criteria +- AC1: Real acceptance criterion +## MUTABLE SECTION +### Plan Version: 1 (Updated: Round 0) +#### Active Tasks +| Task | Target AC | Status | Notes | +|------|-----------|--------|-------| +| Task 1 | AC1 | in_progress | Real task | +EOF +export CLAUDE_PROJECT_DIR="$PWD" +set +e +RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) +EXIT_CODE=$? +set -e +# Should report Ultimate Goal but NOT Acceptance Criteria or Active Tasks +if echo "$RESULT" | grep -q "Ultimate Goal" && \ + ! echo "$RESULT" | grep -q "Acceptance Criteria.*placeholder" && \ + ! echo "$RESULT" | grep -q "Active Tasks.*placeholder"; then + pass "Stop hook only reports Ultimate Goal placeholder" +else + fail "Section-specific Ultimate Goal" "only Ultimate Goal reported" "output: $RESULT" +fi + +# Test 14.2: Stop hook only reports Acceptance Criteria placeholder when only that is missing +echo "Test 14.2: Stop hook only reports Acceptance Criteria placeholder" +cd "$TEST_DIR" +rm -rf placeholder-test-14-2 2>/dev/null || true +mkdir -p placeholder-test-14-2 +cd placeholder-test-14-2 +git init -q +git config user.email "test@test.com" +git config user.name "Test" +echo "init" > init.txt +echo ".humanize*" > .gitignore +git add init.txt .gitignore +git -c commit.gpgsign=false commit -q -m "Initial" +TEST_BRANCH=$(git rev-parse --abbrev-ref HEAD) +mkdir -p plans +echo "plans/" >> .gitignore +cat > plans/test-plan.md << 'EOF' +# Test Plan +## Goal +Test +EOF +git add .gitignore +git -c commit.gpgsign=false commit -q -m "Add gitignore" +LOOP_DIR_14_2="$PWD/.humanize/rlcr/2024-01-01_12-00-00" +mkdir -p "$LOOP_DIR_14_2" +cp plans/test-plan.md "$LOOP_DIR_14_2/plan.md" +cat > "$LOOP_DIR_14_2/state.md" << EOF +--- +current_round: 0 +max_iterations: 42 +plan_file: "plans/test-plan.md" +plan_tracked: false +start_branch: $TEST_BRANCH +--- +EOF +cat > "$LOOP_DIR_14_2/round-0-summary.md" << 'EOF' +# Summary +Work done. +EOF +# Goal tracker with ONLY AC placeholder (Goal and Tasks are filled) +cat > "$LOOP_DIR_14_2/goal-tracker.md" << 'EOF' +# Goal Tracker +## IMMUTABLE SECTION +### Ultimate Goal +Implement the feature completely +### Acceptance Criteria +[To be defined by Claude in Round 0 based on the plan] +## MUTABLE SECTION +### Plan Version: 1 (Updated: Round 0) +#### Active Tasks +| Task | Target AC | Status | Notes | +|------|-----------|--------|-------| +| Task 1 | AC1 | in_progress | Real task | +EOF +export CLAUDE_PROJECT_DIR="$PWD" +set +e +RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) +EXIT_CODE=$? +set -e +# Should report Acceptance Criteria but NOT Ultimate Goal or Active Tasks +if echo "$RESULT" | grep -q "Acceptance Criteria" && \ + ! echo "$RESULT" | grep -q "Ultimate Goal.*placeholder" && \ + ! echo "$RESULT" | grep -q "Active Tasks.*placeholder"; then + pass "Stop hook only reports Acceptance Criteria placeholder" +else + fail "Section-specific Acceptance Criteria" "only Acceptance Criteria reported" "output: $RESULT" +fi + +# Test 14.3: Stop hook only reports Active Tasks placeholder when only that is missing +echo "Test 14.3: Stop hook only reports Active Tasks placeholder" +cd "$TEST_DIR" +rm -rf placeholder-test-14-3 2>/dev/null || true +mkdir -p placeholder-test-14-3 +cd placeholder-test-14-3 +git init -q +git config user.email "test@test.com" +git config user.name "Test" +echo "init" > init.txt +echo ".humanize*" > .gitignore +git add init.txt .gitignore +git -c commit.gpgsign=false commit -q -m "Initial" +TEST_BRANCH=$(git rev-parse --abbrev-ref HEAD) +mkdir -p plans +echo "plans/" >> .gitignore +cat > plans/test-plan.md << 'EOF' +# Test Plan +## Goal +Test +EOF +git add .gitignore +git -c commit.gpgsign=false commit -q -m "Add gitignore" +LOOP_DIR_14_3="$PWD/.humanize/rlcr/2024-01-01_12-00-00" +mkdir -p "$LOOP_DIR_14_3" +cp plans/test-plan.md "$LOOP_DIR_14_3/plan.md" +cat > "$LOOP_DIR_14_3/state.md" << EOF +--- +current_round: 0 +max_iterations: 42 +plan_file: "plans/test-plan.md" +plan_tracked: false +start_branch: $TEST_BRANCH +--- +EOF +cat > "$LOOP_DIR_14_3/round-0-summary.md" << 'EOF' +# Summary +Work done. +EOF +# Goal tracker with ONLY Active Tasks placeholder (Goal and AC are filled) +cat > "$LOOP_DIR_14_3/goal-tracker.md" << 'EOF' +# Goal Tracker +## IMMUTABLE SECTION +### Ultimate Goal +Implement the feature completely +### Acceptance Criteria +- AC1: Real acceptance criterion +## MUTABLE SECTION +### Plan Version: 1 (Updated: Round 0) +#### Active Tasks +[To be populated by Claude based on plan] +EOF +export CLAUDE_PROJECT_DIR="$PWD" +set +e +RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) +EXIT_CODE=$? +set -e +# Should report Active Tasks but NOT Ultimate Goal or Acceptance Criteria +if echo "$RESULT" | grep -q "Active Tasks" && \ + ! echo "$RESULT" | grep -q "Ultimate Goal.*placeholder" && \ + ! echo "$RESULT" | grep -q "Acceptance Criteria.*placeholder"; then + pass "Stop hook only reports Active Tasks placeholder" +else + fail "Section-specific Active Tasks" "only Active Tasks reported" "output: $RESULT" +fi + +# Test 14.4: Stop hook reports all three when all placeholders present +echo "Test 14.4: Stop hook reports all three placeholders when all missing" +cd "$TEST_DIR" +rm -rf placeholder-test-14-4 2>/dev/null || true +mkdir -p placeholder-test-14-4 +cd placeholder-test-14-4 +git init -q +git config user.email "test@test.com" +git config user.name "Test" +echo "init" > init.txt +echo ".humanize*" > .gitignore +git add init.txt .gitignore +git -c commit.gpgsign=false commit -q -m "Initial" +TEST_BRANCH=$(git rev-parse --abbrev-ref HEAD) +mkdir -p plans +echo "plans/" >> .gitignore +cat > plans/test-plan.md << 'EOF' +# Test Plan +## Goal +Test +EOF +git add .gitignore +git -c commit.gpgsign=false commit -q -m "Add gitignore" +LOOP_DIR_14_4="$PWD/.humanize/rlcr/2024-01-01_12-00-00" +mkdir -p "$LOOP_DIR_14_4" +cp plans/test-plan.md "$LOOP_DIR_14_4/plan.md" +cat > "$LOOP_DIR_14_4/state.md" << EOF +--- +current_round: 0 +max_iterations: 42 +plan_file: "plans/test-plan.md" +plan_tracked: false +start_branch: $TEST_BRANCH +--- +EOF +cat > "$LOOP_DIR_14_4/round-0-summary.md" << 'EOF' +# Summary +Work done. +EOF +# Goal tracker with ALL placeholders +cat > "$LOOP_DIR_14_4/goal-tracker.md" << 'EOF' +# Goal Tracker +## IMMUTABLE SECTION +### Ultimate Goal +[To be extracted from plan by Claude in Round 0] +### Acceptance Criteria +[To be defined by Claude in Round 0 based on the plan] +## MUTABLE SECTION +### Plan Version: 1 (Updated: Round 0) +#### Active Tasks +[To be populated by Claude based on plan] +EOF +export CLAUDE_PROJECT_DIR="$PWD" +set +e +RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) +EXIT_CODE=$? +set -e +# Should report all three placeholders +if echo "$RESULT" | grep -q "Ultimate Goal" && \ + echo "$RESULT" | grep -q "Acceptance Criteria" && \ + echo "$RESULT" | grep -q "Active Tasks"; then + pass "Stop hook reports all three placeholders when all missing" +else + fail "All placeholders reported" "all three reported" "output: $RESULT" +fi + echo "" echo "=== Test: Legacy Path Handling (NEGATIVE TESTS) ===" echo "" From 3f453753684d3d8fc8bd3346c785e8964408848e Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Thu, 15 Jan 2026 22:48:00 -0800 Subject: [PATCH 09/17] Fix: Section-scoped placeholder detection using awk extraction Changed placeholder detection from fixed string matching across the entire file to section-scoped detection using awk to extract each section block. Uses generic placeholder pattern `[To be [a-z]` within each section only. - Ultimate Goal: extracts lines after `### Ultimate Goal` until next `##` - Acceptance Criteria: extracts lines after `### Acceptance Criteria` until next `##` - Active Tasks: extracts lines after `#### Active Tasks` until next `##` Tightened tests to assert exact missing-item lines (e.g., `**Ultimate Goal**: Still contains placeholder text`) rather than just section names. --- hooks/loop-codex-stop-hook.sh | 26 ++++++++++++++------- tests/test-plan-file-hooks.sh | 44 +++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 20b288df..28e222c1 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -521,26 +521,34 @@ GOAL_TRACKER_FILE="$LOOP_DIR/goal-tracker.md" if [[ "$CURRENT_ROUND" -eq 0 ]] && [[ -f "$GOAL_TRACKER_FILE" ]]; then # Check if goal-tracker.md still contains placeholder text - GOAL_TRACKER_CONTENT=$(cat "$GOAL_TRACKER_FILE") + # Extract each section and check for generic placeholder pattern within that section + # This avoids coupling to specific placeholder wording and prevents false positives + # from stray mentions of placeholder text elsewhere in the file HAS_GOAL_PLACEHOLDER=false HAS_AC_PLACEHOLDER=false HAS_TASKS_PLACEHOLDER=false - # Use section-specific placeholder patterns to avoid overlap - # Each pattern matches the unique text for that section only - # Ultimate Goal: "[To be extracted from plan by Claude in Round 0]" - if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be extracted from plan'; then + # Extract Ultimate Goal section (### Ultimate Goal to next heading) + # Use awk to extract lines between start and end patterns, excluding end pattern + GOAL_SECTION=$(awk '/^### Ultimate Goal/{found=1; next} /^##/{found=0} found' "$GOAL_TRACKER_FILE" 2>/dev/null) + # Check for generic placeholder pattern "[To be " within this section + if echo "$GOAL_SECTION" | grep -qE '\[To be [a-z]'; then HAS_GOAL_PLACEHOLDER=true fi - # Acceptance Criteria: "[To be defined by Claude in Round 0 based on the plan]" - if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be defined by Claude'; then + # Extract Acceptance Criteria section (### Acceptance Criteria to next heading) + AC_SECTION=$(awk '/^### Acceptance Criteria/{found=1; next} /^##/{found=0} found' "$GOAL_TRACKER_FILE" 2>/dev/null) + # Check for generic placeholder pattern "[To be " within this section + if echo "$AC_SECTION" | grep -qE '\[To be [a-z]'; then HAS_AC_PLACEHOLDER=true fi - # Active Tasks: "[To be populated by Claude based on plan]" - if echo "$GOAL_TRACKER_CONTENT" | grep -qF '[To be populated by Claude'; then + # Extract Active Tasks section (#### Active Tasks to next heading or EOF) + # Active Tasks is a level-4 heading, so match any ## or higher + TASKS_SECTION=$(awk '/^#### Active Tasks/{found=1; next} /^##/{found=0} found' "$GOAL_TRACKER_FILE" 2>/dev/null) + # Check for generic placeholder pattern "[To be " within this section + if echo "$TASKS_SECTION" | grep -qE '\[To be [a-z]'; then HAS_TASKS_PLACEHOLDER=true fi diff --git a/tests/test-plan-file-hooks.sh b/tests/test-plan-file-hooks.sh index 3f2be029..6b69e374 100755 --- a/tests/test-plan-file-hooks.sh +++ b/tests/test-plan-file-hooks.sh @@ -796,13 +796,14 @@ set +e RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) EXIT_CODE=$? set -e -# Should report Ultimate Goal but NOT Acceptance Criteria or Active Tasks -if echo "$RESULT" | grep -q "Ultimate Goal" && \ - ! echo "$RESULT" | grep -q "Acceptance Criteria.*placeholder" && \ - ! echo "$RESULT" | grep -q "Active Tasks.*placeholder"; then +# Should report Ultimate Goal missing-item line but NOT AC or Active Tasks missing-item lines +# The exact format is: **
**: Still contains placeholder text +if echo "$RESULT" | grep -qF '**Ultimate Goal**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Acceptance Criteria**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Active Tasks**: Still contains placeholder text'; then pass "Stop hook only reports Ultimate Goal placeholder" else - fail "Section-specific Ultimate Goal" "only Ultimate Goal reported" "output: $RESULT" + fail "Section-specific Ultimate Goal" "only **Ultimate Goal**: Still contains placeholder text" "output: $RESULT" fi # Test 14.2: Stop hook only reports Acceptance Criteria placeholder when only that is missing @@ -864,13 +865,14 @@ set +e RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) EXIT_CODE=$? set -e -# Should report Acceptance Criteria but NOT Ultimate Goal or Active Tasks -if echo "$RESULT" | grep -q "Acceptance Criteria" && \ - ! echo "$RESULT" | grep -q "Ultimate Goal.*placeholder" && \ - ! echo "$RESULT" | grep -q "Active Tasks.*placeholder"; then +# Should report Acceptance Criteria missing-item line but NOT Goal or Active Tasks missing-item lines +# The exact format is: **
**: Still contains placeholder text +if echo "$RESULT" | grep -qF '**Acceptance Criteria**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Ultimate Goal**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Active Tasks**: Still contains placeholder text'; then pass "Stop hook only reports Acceptance Criteria placeholder" else - fail "Section-specific Acceptance Criteria" "only Acceptance Criteria reported" "output: $RESULT" + fail "Section-specific Acceptance Criteria" "only **Acceptance Criteria**: Still contains placeholder text" "output: $RESULT" fi # Test 14.3: Stop hook only reports Active Tasks placeholder when only that is missing @@ -930,13 +932,14 @@ set +e RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) EXIT_CODE=$? set -e -# Should report Active Tasks but NOT Ultimate Goal or Acceptance Criteria -if echo "$RESULT" | grep -q "Active Tasks" && \ - ! echo "$RESULT" | grep -q "Ultimate Goal.*placeholder" && \ - ! echo "$RESULT" | grep -q "Acceptance Criteria.*placeholder"; then +# Should report Active Tasks missing-item line but NOT Goal or AC missing-item lines +# The exact format is: **
**: Still contains placeholder text +if echo "$RESULT" | grep -qF '**Active Tasks**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Ultimate Goal**: Still contains placeholder text' && \ + ! echo "$RESULT" | grep -qF '**Acceptance Criteria**: Still contains placeholder text'; then pass "Stop hook only reports Active Tasks placeholder" else - fail "Section-specific Active Tasks" "only Active Tasks reported" "output: $RESULT" + fail "Section-specific Active Tasks" "only **Active Tasks**: Still contains placeholder text" "output: $RESULT" fi # Test 14.4: Stop hook reports all three when all placeholders present @@ -996,13 +999,14 @@ set +e RESULT=$(echo '{}' | "$PROJECT_ROOT/hooks/loop-codex-stop-hook.sh" 2>&1) EXIT_CODE=$? set -e -# Should report all three placeholders -if echo "$RESULT" | grep -q "Ultimate Goal" && \ - echo "$RESULT" | grep -q "Acceptance Criteria" && \ - echo "$RESULT" | grep -q "Active Tasks"; then +# Should report all three missing-item lines +# The exact format is: **
**: Still contains placeholder text +if echo "$RESULT" | grep -qF '**Ultimate Goal**: Still contains placeholder text' && \ + echo "$RESULT" | grep -qF '**Acceptance Criteria**: Still contains placeholder text' && \ + echo "$RESULT" | grep -qF '**Active Tasks**: Still contains placeholder text'; then pass "Stop hook reports all three placeholders when all missing" else - fail "All placeholders reported" "all three reported" "output: $RESULT" + fail "All placeholders reported" "all three **
**: Still contains placeholder text lines" "output: $RESULT" fi echo "" From 3d7451f887bbd1ea0582908eb38e5ae89e2dde2b Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 00:03:58 -0800 Subject: [PATCH 10/17] Fix: Fail-closed on git status timeout in stop hook Previously, git status --porcelain failures were swallowed with || echo "", treating timeouts or errors as "clean repo" and allowing exit. This could bypass large-file and uncommitted-changes checks in slow/unhealthy repos. Now captures exit code and blocks exit on non-zero, with templated error msg. --- hooks/loop-codex-stop-hook.sh | 19 ++++++++++++++++++- prompt-template/block/git-status-failed.md | 10 ++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 prompt-template/block/git-status-failed.md diff --git a/hooks/loop-codex-stop-hook.sh b/hooks/loop-codex-stop-hook.sh index 28e222c1..3d324f55 100755 --- a/hooks/loop-codex-stop-hook.sh +++ b/hooks/loop-codex-stop-hook.sh @@ -299,13 +299,30 @@ fi # ======================================== # Cache git status output to avoid calling it multiple times. # Used by both large file check and git clean check below. +# IMPORTANT: Fail-closed on git failures to prevent bypassing checks. GIT_STATUS_CACHED="" GIT_IS_REPO=false if command -v git &>/dev/null && run_with_timeout "$GIT_TIMEOUT" git rev-parse --git-dir &>/dev/null 2>&1; then GIT_IS_REPO=true - GIT_STATUS_CACHED=$(run_with_timeout "$GIT_TIMEOUT" git status --porcelain 2>/dev/null || echo "") + # Capture exit code to detect timeout/failure - do NOT use || echo "" which would fail-open + GIT_STATUS_EXIT=0 + GIT_STATUS_CACHED=$(run_with_timeout "$GIT_TIMEOUT" git status --porcelain 2>/dev/null) || GIT_STATUS_EXIT=$? + + if [[ $GIT_STATUS_EXIT -ne 0 ]]; then + # Git status failed or timed out - fail-closed by blocking exit + FALLBACK="# Git Status Failed + +Git status operation failed or timed out (exit code {{GIT_STATUS_EXIT}}). + +Cannot verify repository state. Please check git status manually and try again." + REASON=$(load_and_render_safe "$TEMPLATE_DIR" "block/git-status-failed.md" "$FALLBACK" \ + "GIT_STATUS_EXIT=$GIT_STATUS_EXIT") + jq -n --arg reason "$REASON" --arg msg "Loop: Blocked - git status failed (exit $GIT_STATUS_EXIT)" \ + '{"decision": "block", "reason": $reason, "systemMessage": $msg}' + exit 0 + fi fi # ======================================== diff --git a/prompt-template/block/git-status-failed.md b/prompt-template/block/git-status-failed.md new file mode 100644 index 00000000..a097a7a9 --- /dev/null +++ b/prompt-template/block/git-status-failed.md @@ -0,0 +1,10 @@ +# Git Status Failed + +Git status operation failed or timed out (exit code {{GIT_STATUS_EXIT}}). + +Cannot verify repository state. This may indicate: +- Git is not responding (possible lock contention) +- Repository is in an invalid state +- Large repository causing slow operations + +Please check git status manually and try again. From 19a8d8750c262c8271866cbe49b39dd2749e0766 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 00:21:43 -0800 Subject: [PATCH 11/17] Fix: ANSI escape code handling in test runner output parsing The run-all-tests.sh script was reporting "0 tests passed" because the regex did not properly handle ANSI escape codes in the test output. The issue: Test output contains "Passed: \x1b[0;32m43\x1b[0m" but the old regex looked for literal "[0-9;m" patterns without the ESC character. The fix: Strip ANSI escape codes using sed before parsing the numbers. This ensures the test runner correctly reports the total test count (currently 357 tests across 9 suites). --- tests/run-all-tests.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/run-all-tests.sh b/tests/run-all-tests.sh index 09f7db53..c3fd7734 100755 --- a/tests/run-all-tests.sh +++ b/tests/run-all-tests.sh @@ -60,8 +60,10 @@ for suite in "${TEST_SUITES[@]}"; do set -e # Extract pass/fail counts from output (look for "Passed: N" pattern) - passed=$(echo "$output" | grep -oE 'Passed:[[:space:]]*\[[0-9;m]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") - failed=$(echo "$output" | grep -oE 'Failed:[[:space:]]*\[[0-9;m]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") + # Strip ANSI escape codes first, then extract the number + # ANSI escape codes are ESC[...m where ESC is \x1b or \033 + passed=$(echo "$output" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") + failed=$(echo "$output" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") # Default to 0 if extraction failed passed=${passed:-0} From b45cd4a584c7c68eaf472f12dd0d4c22bed9ac3a Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 00:30:31 -0800 Subject: [PATCH 12/17] Fix: Portable ANSI escape code handling and add regression tests Changes: 1. Fixed ANSI escape code stripping in run-all-tests.sh to be portable across GNU (Linux) and BSD (macOS) sed implementations. - Changed from \x1b (GNU-specific) to $'\033' (ANSI-C quoting) - Reused output_stripped variable for efficiency - Removed redundant ${var:-0} fallback (|| echo "0" already handles it) 2. Added test-ansi-parsing.sh with 8 regression tests covering: - Basic ANSI color stripping - Multiple colors in one line - Passed/Failed count extraction from colored output - Zero count handling - Complex multi-line output parsing - Plain text (no ANSI codes) handling - Bold+color combined codes 3. Added test-ansi-parsing.sh to the test suite list. Test results: 365 tests passed (357 original + 8 new), 0 failed. --- tests/run-all-tests.sh | 14 +-- tests/test-ansi-parsing.sh | 178 +++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 7 deletions(-) create mode 100755 tests/test-ansi-parsing.sh diff --git a/tests/run-all-tests.sh b/tests/run-all-tests.sh index c3fd7734..8957220a 100755 --- a/tests/run-all-tests.sh +++ b/tests/run-all-tests.sh @@ -40,6 +40,7 @@ TEST_SUITES=( "test-templates-comprehensive.sh" "test-plan-file-hooks.sh" "test-error-scenarios.sh" + "test-ansi-parsing.sh" ) for suite in "${TEST_SUITES[@]}"; do @@ -61,13 +62,12 @@ for suite in "${TEST_SUITES[@]}"; do # Extract pass/fail counts from output (look for "Passed: N" pattern) # Strip ANSI escape codes first, then extract the number - # ANSI escape codes are ESC[...m where ESC is \x1b or \033 - passed=$(echo "$output" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") - failed=$(echo "$output" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") - - # Default to 0 if extraction failed - passed=${passed:-0} - failed=${failed:-0} + # Use $'\033' (ANSI-C quoting) for portability across GNU and BSD sed + # Note: \x1b is GNU sed specific; $'\033' works in bash on both Linux and macOS + esc=$'\033' + output_stripped=$(echo "$output" | sed "s/${esc}\\[[0-9;]*m//g") + passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") + failed=$(echo "$output_stripped" | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") TOTAL_PASSED=$((TOTAL_PASSED + passed)) TOTAL_FAILED=$((TOTAL_FAILED + failed)) diff --git a/tests/test-ansi-parsing.sh b/tests/test-ansi-parsing.sh new file mode 100755 index 00000000..4394a8d7 --- /dev/null +++ b/tests/test-ansi-parsing.sh @@ -0,0 +1,178 @@ +#!/bin/bash +# +# Test ANSI escape code handling in test runner output parsing +# +# This tests the portable ANSI stripping used in run-all-tests.sh +# to ensure it works correctly on both GNU (Linux) and BSD (macOS) sed. +# + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' + +TESTS_PASSED=0 +TESTS_FAILED=0 + +pass() { + echo -e "${GREEN}PASS${NC}: $1" + TESTS_PASSED=$((TESTS_PASSED + 1)) +} + +fail() { + echo -e "${RED}FAIL${NC}: $1" + echo " Expected: $2" + echo " Got: $3" + TESTS_FAILED=$((TESTS_FAILED + 1)) +} + +echo "========================================" +echo "Testing ANSI Escape Code Parsing" +echo "========================================" +echo "" + +# Test the portable ANSI stripping approach used in run-all-tests.sh +# Uses $'\033' (ANSI-C quoting) which works on both GNU and BSD sed + +# ======================================== +# Test 1: Basic ANSI color stripping +# ======================================== +echo "Test 1: Basic ANSI color stripping" +input=$'Passed: \033[0;32m43\033[0m' +esc=$'\033' +result=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +expected="Passed: 43" +if [[ "$result" == "$expected" ]]; then + pass "Basic color stripping works" +else + fail "Basic color stripping" "$expected" "$result" +fi + +# ======================================== +# Test 2: Multiple colors in one line +# ======================================== +echo "" +echo "Test 2: Multiple colors in one line" +input=$'\033[1mBold\033[0m and \033[0;31mRed\033[0m and \033[0;32mGreen\033[0m' +result=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +expected="Bold and Red and Green" +if [[ "$result" == "$expected" ]]; then + pass "Multiple colors stripped correctly" +else + fail "Multiple colors" "$expected" "$result" +fi + +# ======================================== +# Test 3: Extract Passed count from colored output +# ======================================== +echo "" +echo "Test 3: Extract Passed count from colored output" +input=$'Passed: \033[0;32m357\033[0m' +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' || echo "0") +if [[ "$passed" == "357" ]]; then + pass "Passed count extracted correctly" +else + fail "Passed count extraction" "357" "$passed" +fi + +# ======================================== +# Test 4: Extract Failed count from colored output +# ======================================== +echo "" +echo "Test 4: Extract Failed count from colored output" +input=$'Failed: \033[0;31m5\033[0m' +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +failed=$(echo "$output_stripped" | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' || echo "0") +if [[ "$failed" == "5" ]]; then + pass "Failed count extracted correctly" +else + fail "Failed count extraction" "5" "$failed" +fi + +# ======================================== +# Test 5: Zero count extraction +# ======================================== +echo "" +echo "Test 5: Zero count extraction" +input=$'Failed: \033[0;31m0\033[0m' +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +failed=$(echo "$output_stripped" | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' || echo "0") +if [[ "$failed" == "0" ]]; then + pass "Zero count extracted correctly" +else + fail "Zero count extraction" "0" "$failed" +fi + +# ======================================== +# Test 6: Complex multi-line output simulation +# ======================================== +echo "" +echo "Test 6: Complex multi-line output (simulating test suite)" +input=$'======================================== +Test Summary +======================================== +Passed: \033[0;32m43\033[0m +Failed: \033[0;31m2\033[0m + +\033[0;31mSome tests failed!\033[0m' +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") +failed=$(echo "$output_stripped" | grep -oE 'Failed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") +if [[ "$passed" == "43" && "$failed" == "2" ]]; then + pass "Complex multi-line output parsed correctly" +else + fail "Complex multi-line output" "passed=43, failed=2" "passed=$passed, failed=$failed" +fi + +# ======================================== +# Test 7: No ANSI codes (plain text) +# ======================================== +echo "" +echo "Test 7: No ANSI codes (plain text)" +input="Passed: 100" +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' || echo "0") +if [[ "$passed" == "100" ]]; then + pass "Plain text without ANSI codes works" +else + fail "Plain text parsing" "100" "$passed" +fi + +# ======================================== +# Test 8: Bold and color combined +# ======================================== +echo "" +echo "Test 8: Bold and color combined" +input=$'\033[1;32mPassed: 50\033[0m' +output_stripped=$(echo "$input" | sed "s/${esc}\\[[0-9;]*m//g") +passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' || echo "0") +if [[ "$passed" == "50" ]]; then + pass "Bold+color combined works" +else + fail "Bold+color combined" "50" "$passed" +fi + +# ======================================== +# Summary +# ======================================== +echo "" +echo "========================================" +echo "Test Summary" +echo "========================================" +echo -e "Passed: ${GREEN}$TESTS_PASSED${NC}" +echo -e "Failed: ${RED}$TESTS_FAILED${NC}" + +if [[ $TESTS_FAILED -eq 0 ]]; then + echo "" + echo -e "${GREEN}All tests passed!${NC}" + exit 0 +else + echo "" + echo -e "${RED}Some tests failed!${NC}" + exit 1 +fi From 1ed1828d55f74fb3b7c1eebea39fe4a826edd8d0 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 00:48:39 -0800 Subject: [PATCH 13/17] Refactor: Convert test-error-scenarios.sh to assertion-based tests Changed from exploratory/documentation script to proper assertion-based tests with pass/fail counting: - Added 12 structured tests covering error scenarios - Tests now use pass/fail functions matching other test files - Reports test counts in standard format for run-all-tests.sh parsing - Removed redundant set +e/set -e pairs (script uses set -uo pipefail) - Applied code-simplifier suggestions for cleaner conditionals Test coverage now includes: 1. Template file not found 2. Template directory not found 3. load_and_render with missing template 4. render_template with empty content 5. Special regex characters in values 6. Strict mode (set -euo pipefail) behavior 7-8. load_and_render_safe fallback behavior 9-10. validate_template_dir validation 11-12. Edge cases (empty/unclosed placeholders) Test results: 377 tests passed (365 previous + 12 new), 0 failed. --- tests/test-error-scenarios.sh | 231 +++++++++++++++++++++++++--------- 1 file changed, 169 insertions(+), 62 deletions(-) diff --git a/tests/test-error-scenarios.sh b/tests/test-error-scenarios.sh index 31ac4915..55fbff9c 100755 --- a/tests/test-error-scenarios.sh +++ b/tests/test-error-scenarios.sh @@ -2,7 +2,8 @@ # # Test error scenarios for template-loader.sh # -# This tests what happens when things go wrong. +# These tests verify that error conditions are handled gracefully +# without crashing or producing unexpected behavior. # set -uo pipefail @@ -13,81 +14,104 @@ source "$PROJECT_ROOT/hooks/lib/template-loader.sh" TEMPLATE_DIR=$(get_template_dir "$PROJECT_ROOT/hooks/lib") +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' + +TESTS_PASSED=0 +TESTS_FAILED=0 + +pass() { + echo -e "${GREEN}PASS${NC}: $1" + TESTS_PASSED=$((TESTS_PASSED + 1)) +} + +fail() { + echo -e "${RED}FAIL${NC}: $1" + echo " Expected: $2" + echo " Got: $3" + TESTS_FAILED=$((TESTS_FAILED + 1)) +} + echo "========================================" echo "Testing Error Scenarios" echo "========================================" echo "" # ======================================== -# Scenario 1: Template file not found +# Test 1: Template file not found returns empty # ======================================== -echo "Scenario 1: Template file not found" -set +e # Temporarily disable exit on error -CONTENT=$(load_template "$TEMPLATE_DIR" "non-existing-file.md" 2>&1) +echo "Test 1: Template file not found returns empty" +CONTENT=$(load_template "$TEMPLATE_DIR" "non-existing-file.md" 2>/dev/null) EXIT_CODE=$? -set -e -echo " Exit code: $EXIT_CODE" -echo " Content: '$CONTENT'" -echo " Result: Template not found returns empty (safe)" -echo "" +if [[ -z "$CONTENT" && $EXIT_CODE -eq 0 ]]; then + pass "Template not found returns empty string without error" +else + fail "Template not found handling" "empty string, exit 0" "content='$CONTENT', exit=$EXIT_CODE" +fi # ======================================== -# Scenario 2: Template directory not found +# Test 2: Template directory not found returns empty # ======================================== -echo "Scenario 2: Template directory not found" -set +e -CONTENT=$(load_template "/non/existing/path" "block/git-push.md" 2>&1) -EXIT_CODE=$? -set -e -echo " Exit code: $EXIT_CODE" -echo " Content: '$CONTENT'" -echo " Result: Directory not found returns empty (safe)" echo "" +echo "Test 2: Template directory not found returns empty" +CONTENT=$(load_template "/non/existing/path" "block/git-push.md" 2>/dev/null) +EXIT_CODE=$? +if [[ -z "$CONTENT" && $EXIT_CODE -eq 0 ]]; then + pass "Directory not found returns empty string without error" +else + fail "Directory not found handling" "empty string, exit 0" "content='$CONTENT', exit=$EXIT_CODE" +fi # ======================================== -# Scenario 3: load_and_render with missing template +# Test 3: load_and_render with missing template returns empty # ======================================== -echo "Scenario 3: load_and_render with missing template" -set +e -RESULT=$(load_and_render "$TEMPLATE_DIR" "non-existing.md" "VAR=value" 2>&1) -EXIT_CODE=$? -set -e -echo " Exit code: $EXIT_CODE" -echo " Result: '$RESULT'" -echo " Result: Returns empty (safe)" echo "" +echo "Test 3: load_and_render with missing template returns empty" +RESULT=$(load_and_render "$TEMPLATE_DIR" "non-existing.md" "VAR=value" 2>/dev/null) +EXIT_CODE=$? +if [[ -z "$RESULT" && $EXIT_CODE -eq 0 ]]; then + pass "load_and_render with missing template returns empty" +else + fail "load_and_render missing template" "empty string, exit 0" "result='$RESULT', exit=$EXIT_CODE" +fi # ======================================== -# Scenario 4: render_template with empty content +# Test 4: render_template with empty content returns empty # ======================================== -echo "Scenario 4: render_template with empty content" -set +e +echo "" +echo "Test 4: render_template with empty content returns empty" RESULT=$(render_template "" "VAR=value") EXIT_CODE=$? -set -e -echo " Exit code: $EXIT_CODE" -echo " Result: '$RESULT'" -echo " Result: Returns empty (safe)" -echo "" +if [[ -z "$RESULT" && $EXIT_CODE -eq 0 ]]; then + pass "render_template with empty content returns empty" +else + fail "render_template empty content" "empty string, exit 0" "result='$RESULT', exit=$EXIT_CODE" +fi # ======================================== -# Scenario 5: Variable with special regex characters +# Test 5: Variable with special regex characters renders correctly # ======================================== -echo "Scenario 5: Variable with special regex characters" -set +e -TEMPLATE="Path: {{PATH}}" -RESULT=$(render_template "$TEMPLATE" "PATH=/home/user/file.md [test] (foo) *bar*") -EXIT_CODE=$? -set -e -echo " Exit code: $EXIT_CODE" -echo " Result: '$RESULT'" echo "" +echo "Test 5: Variable with special regex characters" +TEMPLATE="Path: {{PATH}}" +SPECIAL_VALUE="/home/user/file.md [test] (foo) *bar*" +RESULT=$(render_template "$TEMPLATE" "PATH=$SPECIAL_VALUE") +EXPECTED="Path: $SPECIAL_VALUE" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "Special regex characters in value render correctly" +else + fail "Special regex characters" "$EXPECTED" "$RESULT" +fi # ======================================== -# Scenario 6: What happens with set -e? +# Test 6: Script continues with set -euo pipefail and missing template # ======================================== -echo "Scenario 6: Testing with set -euo pipefail" -bash -c ' +echo "" +echo "Test 6: Script continues with set -euo pipefail" +set +e +SCRIPT_OUTPUT=$(bash -c ' set -euo pipefail source "'"$PROJECT_ROOT"'/hooks/lib/template-loader.sh" TEMPLATE_DIR=$(get_template_dir "'"$PROJECT_ROOT"'/hooks/lib") @@ -96,26 +120,109 @@ TEMPLATE_DIR=$(get_template_dir "'"$PROJECT_ROOT"'/hooks/lib") REASON=$(load_and_render "$TEMPLATE_DIR" "non-existing.md" "VAR=value" 2>/dev/null) if [[ -z "$REASON" ]]; then - echo " REASON is empty - script continues, no crash" + echo "EMPTY_REASON" +fi +echo "SCRIPT_COMPLETED" +' 2>&1) +EXIT_CODE=$? +set -e +if [[ "$SCRIPT_OUTPUT" == *"SCRIPT_COMPLETED"* && $EXIT_CODE -eq 0 ]]; then + pass "Script continues without crashing under strict mode" +else + fail "Strict mode handling" "SCRIPT_COMPLETED in output, exit 0" "output='$SCRIPT_OUTPUT', exit=$EXIT_CODE" +fi + +# ======================================== +# Test 7: load_and_render_safe with missing template uses fallback +# ======================================== +echo "" +echo "Test 7: load_and_render_safe uses fallback for missing template" +FALLBACK="This is the fallback message" +RESULT=$(load_and_render_safe "$TEMPLATE_DIR" "non-existing.md" "$FALLBACK" 2>/dev/null) +if [[ "$RESULT" == "$FALLBACK" ]]; then + pass "load_and_render_safe uses fallback correctly" else - echo " REASON has content: $REASON" + fail "load_and_render_safe fallback" "$FALLBACK" "$RESULT" fi -echo " Script reached end without crashing" -' 2>&1 + +# ======================================== +# Test 8: load_and_render_safe with fallback containing variables +# ======================================== echo "" +echo "Test 8: load_and_render_safe fallback with variable substitution" +FALLBACK="Error for {{FILE}}: not found" +RESULT=$(load_and_render_safe "$TEMPLATE_DIR" "non-existing.md" "$FALLBACK" "FILE=test.md" 2>/dev/null) +EXPECTED="Error for test.md: not found" +if [[ "$RESULT" == "$EXPECTED" ]]; then + pass "load_and_render_safe substitutes variables in fallback" +else + fail "load_and_render_safe fallback substitution" "$EXPECTED" "$RESULT" +fi # ======================================== -# Summary +# Test 9: validate_template_dir with valid directory returns 0 +# ======================================== +echo "" +echo "Test 9: validate_template_dir accepts valid directory" +if validate_template_dir "$TEMPLATE_DIR" 2>/dev/null; then + pass "validate_template_dir accepts valid directory" +else + fail "validate_template_dir valid" "exit 0" "exit 1" +fi + +# ======================================== +# Test 10: validate_template_dir with invalid directory returns 1 +# ======================================== +echo "" +echo "Test 10: validate_template_dir rejects invalid directory" +if ! validate_template_dir "/non/existing/path" 2>/dev/null; then + pass "validate_template_dir rejects invalid directory" +else + fail "validate_template_dir invalid" "exit 1" "exit 0" +fi + +# ======================================== +# Test 11: Empty variable name in template stays as-is # ======================================== -echo "========================================" -echo "Error Handling Summary" -echo "========================================" echo "" -echo "Current behavior:" -echo " - Missing template file -> returns empty string" -echo " - Missing directory -> returns empty string" -echo " - Empty content -> returns empty string" -echo " - All cases: exit code 0 (no crash)" +echo "Test 11: Empty placeholder {{}} stays as-is" +TEMPLATE="Test: {{}}" +RESULT=$(render_template "$TEMPLATE" "VAR=value") +if [[ "$RESULT" == "Test: {{}}" ]]; then + pass "Empty placeholder stays unchanged" +else + fail "Empty placeholder handling" "Test: {{}}" "$RESULT" +fi + +# ======================================== +# Test 12: Unclosed placeholder {{ stays as-is +# ======================================== echo "" -echo "RISK: If REASON is empty, Claude receives empty feedback!" +echo "Test 12: Unclosed placeholder {{ stays as-is" +TEMPLATE="Test: {{UNCLOSED" +RESULT=$(render_template "$TEMPLATE" "UNCLOSED=value") +if [[ "$RESULT" == "Test: {{UNCLOSED" ]]; then + pass "Unclosed placeholder stays unchanged" +else + fail "Unclosed placeholder handling" "Test: {{UNCLOSED" "$RESULT" +fi + +# ======================================== +# Summary +# ======================================== echo "" +echo "========================================" +echo "Test Summary" +echo "========================================" +echo -e "Passed: ${GREEN}$TESTS_PASSED${NC}" +echo -e "Failed: ${RED}$TESTS_FAILED${NC}" + +if [[ $TESTS_FAILED -eq 0 ]]; then + echo "" + echo -e "${GREEN}All tests passed!${NC}" + exit 0 +else + echo "" + echo -e "${RED}Some tests failed!${NC}" + exit 1 +fi From 1820595470fe657fe5006fae9cbb5b973373ba5b Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 01:05:40 -0800 Subject: [PATCH 14/17] Fix: Remove incorrect set -e toggle in test-error-scenarios.sh Test 6 The script uses `set -uo pipefail` (without -e), but Test 6 was using `set +e` before and `set -e` after a subshell, incorrectly enabling errexit which was never originally enabled. Changes: - Replaced set +e/set -e with `|| true` after command substitution - Removed unused EXIT_CODE variable (|| true always returns 0) - Simplified test condition to only check for SCRIPT_COMPLETED output - Added clarifying comment about the || true purpose The test verifies that scripts using strict mode can safely call template-loader functions without crashing. Test results: 377 tests passed, 0 failed. --- tests/test-error-scenarios.sh | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/test-error-scenarios.sh b/tests/test-error-scenarios.sh index 55fbff9c..c05c1d8a 100755 --- a/tests/test-error-scenarios.sh +++ b/tests/test-error-scenarios.sh @@ -110,7 +110,7 @@ fi # ======================================== echo "" echo "Test 6: Script continues with set -euo pipefail" -set +e +# Run in isolated subshell - use || true to capture output even if subshell fails SCRIPT_OUTPUT=$(bash -c ' set -euo pipefail source "'"$PROJECT_ROOT"'/hooks/lib/template-loader.sh" @@ -123,13 +123,11 @@ if [[ -z "$REASON" ]]; then echo "EMPTY_REASON" fi echo "SCRIPT_COMPLETED" -' 2>&1) -EXIT_CODE=$? -set -e -if [[ "$SCRIPT_OUTPUT" == *"SCRIPT_COMPLETED"* && $EXIT_CODE -eq 0 ]]; then +' 2>&1) || true +if [[ "$SCRIPT_OUTPUT" == *"SCRIPT_COMPLETED"* ]]; then pass "Script continues without crashing under strict mode" else - fail "Strict mode handling" "SCRIPT_COMPLETED in output, exit 0" "output='$SCRIPT_OUTPUT', exit=$EXIT_CODE" + fail "Strict mode handling" "SCRIPT_COMPLETED in output" "output='$SCRIPT_OUTPUT'" fi # ======================================== From 536f74cced790a0457f3b82dde6f991f6ef66389 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 01:43:59 -0800 Subject: [PATCH 15/17] Feature: Add allowlist for required RLCR round files Add is_allowlisted_file() helper to permit specific round files that are required by Codex review process: - round-1-todos.md, round-2-todos.md: Persisted todos lists - round-0-summary.md, round-1-summary.md: Historical accuracy updates Update all validator hooks (read, write, edit, bash) to check allowlist before blocking access to these specific files. Files modified: - hooks/lib/loop-common.sh: Add is_allowlisted_file() function - hooks/loop-read-validator.sh: Check allowlist for todos and summaries - hooks/loop-write-validator.sh: Check allowlist for todos and summaries - hooks/loop-edit-validator.sh: Check allowlist for todos and summaries - hooks/loop-bash-validator.sh: Check allowlist for round-1/2-todos.md --- hooks/lib/loop-common.sh | 23 +++++++++++++++++++++++ hooks/loop-bash-validator.sh | 6 ++++-- hooks/loop-edit-validator.sh | 17 +++++++++++------ hooks/loop-read-validator.sh | 17 +++++++++++------ hooks/loop-write-validator.sh | 18 ++++++++++++------ 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/hooks/lib/loop-common.sh b/hooks/lib/loop-common.sh index 1f29cdfd..2427d6bf 100755 --- a/hooks/lib/loop-common.sh +++ b/hooks/lib/loop-common.sh @@ -162,6 +162,29 @@ extract_round_number() { echo "$filename_lower" | sed -n 's/.*round-\([0-9][0-9]*\)-\(summary\|prompt\|todos\)\.md$/\1/p' } +# Check if a file is in the allowlist for the active loop +# Usage: is_allowlisted_file "$file_path" "$active_loop_dir" +# Returns: 0 if allowlisted, 1 otherwise +is_allowlisted_file() { + local file_path="$1" + local active_loop_dir="$2" + + local allowlist=( + "round-1-todos.md" + "round-2-todos.md" + "round-0-summary.md" + "round-1-summary.md" + ) + + for allowed in "${allowlist[@]}"; do + if [[ "$file_path" == "$active_loop_dir/$allowed" ]]; then + return 0 + fi + done + + return 1 +} + # Standard message for blocking todos file access # Usage: todos_blocked_message "Read|Write|Bash" todos_blocked_message() { diff --git a/hooks/loop-bash-validator.sh b/hooks/loop-bash-validator.sh index 4604adaf..8da1866c 100755 --- a/hooks/loop-bash-validator.sh +++ b/hooks/loop-bash-validator.sh @@ -134,8 +134,10 @@ fi # ======================================== if command_modifies_file "$COMMAND_LOWER" "round-[0-9]+-todos\.md"; then - todos_blocked_message "Bash" >&2 - exit 2 + if ! echo "$COMMAND_LOWER" | grep -qE "round-[12]-todos\.md"; then + todos_blocked_message "Bash" >&2 + exit 2 + fi fi exit 0 diff --git a/hooks/loop-edit-validator.sh b/hooks/loop-edit-validator.sh index 01aea848..031e090e 100755 --- a/hooks/loop-edit-validator.sh +++ b/hooks/loop-edit-validator.sh @@ -34,8 +34,13 @@ FILE_PATH_LOWER=$(to_lower "$FILE_PATH") # ======================================== if is_round_file_type "$FILE_PATH_LOWER" "todos"; then - todos_blocked_message "Edit" >&2 - exit 2 + PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" + LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" + LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") + if [[ -z "$LOOP_DIR" ]] || ! is_allowlisted_file "$FILE_PATH" "$LOOP_DIR"; then + todos_blocked_message "Edit" >&2 + exit 2 + fi fi if is_round_file_type "$FILE_PATH_LOWER" "prompt"; then @@ -55,9 +60,9 @@ fi # Find Active Loop and Current Round # ======================================== -PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" -LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" -ACTIVE_LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") +PROJECT_ROOT="${PROJECT_ROOT:-${CLAUDE_PROJECT_DIR:-$(pwd)}}" +LOOP_BASE_DIR="${LOOP_BASE_DIR:-$PROJECT_ROOT/.humanize/rlcr}" +ACTIVE_LOOP_DIR="${LOOP_DIR:-$(find_active_loop "$LOOP_BASE_DIR")}" if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 @@ -114,7 +119,7 @@ if is_round_file_type "$FILE_PATH_LOWER" "summary"; then if [[ -n "$CLAUDE_FILENAME" ]]; then CLAUDE_ROUND=$(extract_round_number "$CLAUDE_FILENAME") - if [[ -n "$CLAUDE_ROUND" ]] && [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]]; then + if [[ -n "$CLAUDE_ROUND" ]] && [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]] && ! is_allowlisted_file "$FILE_PATH" "$ACTIVE_LOOP_DIR"; then CORRECT_PATH="$ACTIVE_LOOP_DIR/round-${CURRENT_ROUND}-summary.md" FALLBACK="# Wrong Round Number diff --git a/hooks/loop-read-validator.sh b/hooks/loop-read-validator.sh index 945f2ea0..da22ca31 100755 --- a/hooks/loop-read-validator.sh +++ b/hooks/loop-read-validator.sh @@ -34,8 +34,13 @@ FILE_PATH_LOWER=$(to_lower "$FILE_PATH") # ======================================== if is_round_file_type "$FILE_PATH_LOWER" "todos"; then - todos_blocked_message "Read" >&2 - exit 2 + PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" + LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" + LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") + if [[ -z "$LOOP_DIR" ]] || ! is_allowlisted_file "$FILE_PATH" "$LOOP_DIR"; then + todos_blocked_message "Read" >&2 + exit 2 + fi fi # ======================================== @@ -53,9 +58,9 @@ IN_HUMANIZE_LOOP_DIR=$(is_in_humanize_loop_dir "$FILE_PATH" && echo "true" || ec # Find Active Loop and Current Round # ======================================== -PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" -LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" -ACTIVE_LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") +PROJECT_ROOT="${PROJECT_ROOT:-${CLAUDE_PROJECT_DIR:-$(pwd)}}" +LOOP_BASE_DIR="${LOOP_BASE_DIR:-$PROJECT_ROOT/.humanize/rlcr}" +ACTIVE_LOOP_DIR="${LOOP_DIR:-$(find_active_loop "$LOOP_BASE_DIR")}" if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 @@ -102,7 +107,7 @@ fi # Validate Round Number # ======================================== -if [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]]; then +if [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]] && ! is_allowlisted_file "$FILE_PATH" "$ACTIVE_LOOP_DIR"; then FALLBACK="# Wrong Round File You tried to read round-{{CLAUDE_ROUND}}-{{FILE_TYPE}}.md but current round is **{{CURRENT_ROUND}}**. diff --git a/hooks/loop-write-validator.sh b/hooks/loop-write-validator.sh index 88543f5c..7a802481 100755 --- a/hooks/loop-write-validator.sh +++ b/hooks/loop-write-validator.sh @@ -35,8 +35,13 @@ FILE_PATH_LOWER=$(to_lower "$FILE_PATH") # ======================================== if is_round_file_type "$FILE_PATH_LOWER" "todos"; then - todos_blocked_message "Write" >&2 - exit 2 + PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" + LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" + LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") + if [[ -z "$LOOP_DIR" ]] || ! is_allowlisted_file "$FILE_PATH" "$LOOP_DIR"; then + todos_blocked_message "Write" >&2 + exit 2 + fi fi if is_round_file_type "$FILE_PATH_LOWER" "prompt"; then @@ -70,9 +75,10 @@ fi # Find Active Loop and Current Round # ======================================== -PROJECT_ROOT="${CLAUDE_PROJECT_DIR:-$(pwd)}" -LOOP_BASE_DIR="$PROJECT_ROOT/.humanize/rlcr" -ACTIVE_LOOP_DIR=$(find_active_loop "$LOOP_BASE_DIR") +# Re-initialize if not set by earlier todos check +PROJECT_ROOT="${PROJECT_ROOT:-${CLAUDE_PROJECT_DIR:-$(pwd)}}" +LOOP_BASE_DIR="${LOOP_BASE_DIR:-$PROJECT_ROOT/.humanize/rlcr}" +ACTIVE_LOOP_DIR="${LOOP_DIR:-$(find_active_loop "$LOOP_BASE_DIR")}" if [[ -z "$ACTIVE_LOOP_DIR" ]]; then exit 0 @@ -147,7 +153,7 @@ fi if [[ "$IS_SUMMARY_FILE" == "true" ]]; then CLAUDE_ROUND=$(extract_round_number "$CLAUDE_FILENAME") - if [[ -n "$CLAUDE_ROUND" ]] && [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]]; then + if [[ -n "$CLAUDE_ROUND" ]] && [[ "$CLAUDE_ROUND" != "$CURRENT_ROUND" ]] && ! is_allowlisted_file "$FILE_PATH" "$ACTIVE_LOOP_DIR"; then CORRECT_PATH="$ACTIVE_LOOP_DIR/round-${CURRENT_ROUND}-summary.md" FALLBACK="# Wrong Round Number From 562538fdfdf25f69c18396edd92e1acbb94675cd Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 01:58:40 -0800 Subject: [PATCH 16/17] Feature: Tighten allowlist and add validator tests Improvements: - Bash validator: Require active loop dir path for round-1/2-todos.md (fixes permissive allowlist that allowed any path) - Add test-allowlist-validators.sh with 24 tests covering: - is_allowlisted_file() function - Read/Write/Edit validator allowlist behavior - Bash validator path-restricted allowlist - Add test-allowlist-validators.sh to run-all-tests.sh Test results: 401 tests passed across 11 suites Files modified: - hooks/loop-bash-validator.sh: Tighten allowlist path check - tests/run-all-tests.sh: Add test-allowlist-validators.sh - tests/test-allowlist-validators.sh: New test file (24 tests) --- hooks/loop-bash-validator.sh | 3 +- tests/run-all-tests.sh | 6 +- tests/test-allowlist-validators.sh | 376 +++++++++++++++++++++++++++++ 3 files changed, 380 insertions(+), 5 deletions(-) create mode 100755 tests/test-allowlist-validators.sh diff --git a/hooks/loop-bash-validator.sh b/hooks/loop-bash-validator.sh index 8da1866c..39702c9a 100755 --- a/hooks/loop-bash-validator.sh +++ b/hooks/loop-bash-validator.sh @@ -134,7 +134,8 @@ fi # ======================================== if command_modifies_file "$COMMAND_LOWER" "round-[0-9]+-todos\.md"; then - if ! echo "$COMMAND_LOWER" | grep -qE "round-[12]-todos\.md"; then + ACTIVE_LOOP_DIRNAME=$(basename "$ACTIVE_LOOP_DIR") + if ! echo "$COMMAND_LOWER" | grep -qE "\.humanize/rlcr/${ACTIVE_LOOP_DIRNAME}/round-[12]-todos\.md"; then todos_blocked_message "Bash" >&2 exit 2 fi diff --git a/tests/run-all-tests.sh b/tests/run-all-tests.sh index 8957220a..36e7c89f 100755 --- a/tests/run-all-tests.sh +++ b/tests/run-all-tests.sh @@ -41,6 +41,7 @@ TEST_SUITES=( "test-plan-file-hooks.sh" "test-error-scenarios.sh" "test-ansi-parsing.sh" + "test-allowlist-validators.sh" ) for suite in "${TEST_SUITES[@]}"; do @@ -60,10 +61,7 @@ for suite in "${TEST_SUITES[@]}"; do exit_code=$? set -e - # Extract pass/fail counts from output (look for "Passed: N" pattern) - # Strip ANSI escape codes first, then extract the number - # Use $'\033' (ANSI-C quoting) for portability across GNU and BSD sed - # Note: \x1b is GNU sed specific; $'\033' works in bash on both Linux and macOS + # Strip ANSI escape codes and extract pass/fail counts esc=$'\033' output_stripped=$(echo "$output" | sed "s/${esc}\\[[0-9;]*m//g") passed=$(echo "$output_stripped" | grep -oE 'Passed:[[:space:]]*[0-9]+' | grep -oE '[0-9]+$' | tail -1 || echo "0") diff --git a/tests/test-allowlist-validators.sh b/tests/test-allowlist-validators.sh new file mode 100755 index 00000000..453631c6 --- /dev/null +++ b/tests/test-allowlist-validators.sh @@ -0,0 +1,376 @@ +#!/bin/bash +# +# Tests for allowlist behavior in RLCR loop validators +# +# Tests: +# - is_allowlisted_file() function in loop-common.sh +# - Read validator allowlist for todos and summaries +# - Write validator allowlist for todos and summaries +# - Edit validator allowlist for todos and summaries +# - Bash validator allowlist for todos files (path-restricted) +# + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +source "$PROJECT_ROOT/hooks/lib/loop-common.sh" + +# Test helpers +GREEN='\033[0;32m' +RED='\033[0;31m' +NC='\033[0m' +TESTS_PASSED=0 +TESTS_FAILED=0 + +pass() { echo -e "${GREEN}PASS${NC}: $1"; TESTS_PASSED=$((TESTS_PASSED + 1)); } +fail() { echo -e "${RED}FAIL${NC}: $1"; echo " Expected: $2"; echo " Got: $3"; TESTS_FAILED=$((TESTS_FAILED + 1)); } + +# Setup test environment +TEST_DIR=$(mktemp -d) +trap "rm -rf $TEST_DIR" EXIT + +setup_test_loop() { + cd "$TEST_DIR" + + if [[ ! -d ".git" ]]; then + git init -q + git config user.email "test@test.com" + git config user.name "Test" + echo "initial" > init.txt + git add init.txt + git -c commit.gpgsign=false commit -q -m "Initial commit" + fi + + local current_branch + current_branch=$(git rev-parse --abbrev-ref HEAD) + + # Create loop directory structure + LOOP_DIR="$TEST_DIR/.humanize/rlcr/2024-01-01_12-00-00" + mkdir -p "$LOOP_DIR" + + # Create state file + cat > "$LOOP_DIR/state.md" << EOF +--- +current_round: 5 +max_iterations: 42 +plan_file: "plans/test-plan.md" +plan_tracked: false +start_branch: $current_branch +--- +EOF +} + +echo "=== Test: is_allowlisted_file() Function ===" +echo "" + +setup_test_loop +ACTIVE_LOOP_DIR="$LOOP_DIR" + +# Test 1: Allowlisted file - round-1-todos.md +echo "Test 1: round-1-todos.md is allowlisted" +if is_allowlisted_file "$ACTIVE_LOOP_DIR/round-1-todos.md" "$ACTIVE_LOOP_DIR"; then + pass "round-1-todos.md is allowlisted" +else + fail "round-1-todos.md allowlist" "true" "false" +fi + +# Test 2: Allowlisted file - round-2-todos.md +echo "Test 2: round-2-todos.md is allowlisted" +if is_allowlisted_file "$ACTIVE_LOOP_DIR/round-2-todos.md" "$ACTIVE_LOOP_DIR"; then + pass "round-2-todos.md is allowlisted" +else + fail "round-2-todos.md allowlist" "true" "false" +fi + +# Test 3: Allowlisted file - round-0-summary.md +echo "Test 3: round-0-summary.md is allowlisted" +if is_allowlisted_file "$ACTIVE_LOOP_DIR/round-0-summary.md" "$ACTIVE_LOOP_DIR"; then + pass "round-0-summary.md is allowlisted" +else + fail "round-0-summary.md allowlist" "true" "false" +fi + +# Test 4: Allowlisted file - round-1-summary.md +echo "Test 4: round-1-summary.md is allowlisted" +if is_allowlisted_file "$ACTIVE_LOOP_DIR/round-1-summary.md" "$ACTIVE_LOOP_DIR"; then + pass "round-1-summary.md is allowlisted" +else + fail "round-1-summary.md allowlist" "true" "false" +fi + +# Test 5: Non-allowlisted file - round-3-todos.md +echo "Test 5: round-3-todos.md is NOT allowlisted" +if ! is_allowlisted_file "$ACTIVE_LOOP_DIR/round-3-todos.md" "$ACTIVE_LOOP_DIR"; then + pass "round-3-todos.md is NOT allowlisted" +else + fail "round-3-todos.md blocked" "false" "true" +fi + +# Test 6: Non-allowlisted file - round-2-summary.md +echo "Test 6: round-2-summary.md is NOT allowlisted" +if ! is_allowlisted_file "$ACTIVE_LOOP_DIR/round-2-summary.md" "$ACTIVE_LOOP_DIR"; then + pass "round-2-summary.md is NOT allowlisted" +else + fail "round-2-summary.md blocked" "false" "true" +fi + +# Test 7: Wrong directory - allowlisted filename but wrong path +echo "Test 7: round-1-todos.md in wrong directory is NOT allowlisted" +if ! is_allowlisted_file "/other/path/round-1-todos.md" "$ACTIVE_LOOP_DIR"; then + pass "round-1-todos.md in wrong directory is blocked" +else + fail "wrong directory check" "false" "true" +fi + +echo "" +echo "=== Test: Write Validator Allowlist ===" +echo "" + +setup_test_loop +export CLAUDE_PROJECT_DIR="$TEST_DIR" + +# Test 8: Write validator allows round-1-todos.md in active loop dir +echo "Test 8: Write validator allows round-1-todos.md" +HOOK_INPUT='{"tool_name": "Write", "tool_input": {"file_path": "'$LOOP_DIR'/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-write-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Write validator allows round-1-todos.md" +else + fail "Write validator round-1-todos.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 9: Write validator allows round-0-summary.md (non-current round) +echo "Test 9: Write validator allows round-0-summary.md (historical)" +HOOK_INPUT='{"tool_name": "Write", "tool_input": {"file_path": "'$LOOP_DIR'/round-0-summary.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-write-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Write validator allows round-0-summary.md" +else + fail "Write validator round-0-summary.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 10: Write validator blocks round-3-todos.md (not in allowlist) +echo "Test 10: Write validator blocks round-3-todos.md" +HOOK_INPUT='{"tool_name": "Write", "tool_input": {"file_path": "'$LOOP_DIR'/round-3-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-write-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Write validator blocks round-3-todos.md" +else + fail "Write validator round-3-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 11: Write validator blocks round-2-summary.md (not in allowlist) +echo "Test 11: Write validator blocks round-2-summary.md" +HOOK_INPUT='{"tool_name": "Write", "tool_input": {"file_path": "'$LOOP_DIR'/round-2-summary.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-write-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "round"; then + pass "Write validator blocks round-2-summary.md" +else + fail "Write validator round-2-summary.md" "exit 2 with round error" "exit $EXIT_CODE, output: $RESULT" +fi + +echo "" +echo "=== Test: Edit Validator Allowlist ===" +echo "" + +# Test 12: Edit validator allows round-2-todos.md in active loop dir +echo "Test 12: Edit validator allows round-2-todos.md" +HOOK_INPUT='{"tool_name": "Edit", "tool_input": {"file_path": "'$LOOP_DIR'/round-2-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-edit-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Edit validator allows round-2-todos.md" +else + fail "Edit validator round-2-todos.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 13: Edit validator allows round-1-summary.md (historical) +echo "Test 13: Edit validator allows round-1-summary.md (historical)" +HOOK_INPUT='{"tool_name": "Edit", "tool_input": {"file_path": "'$LOOP_DIR'/round-1-summary.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-edit-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Edit validator allows round-1-summary.md" +else + fail "Edit validator round-1-summary.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 14: Edit validator blocks round-4-todos.md +echo "Test 14: Edit validator blocks round-4-todos.md" +HOOK_INPUT='{"tool_name": "Edit", "tool_input": {"file_path": "'$LOOP_DIR'/round-4-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-edit-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Edit validator blocks round-4-todos.md" +else + fail "Edit validator round-4-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +echo "" +echo "=== Test: Read Validator Allowlist ===" +echo "" + +# Test 15: Read validator allows round-1-todos.md +echo "Test 15: Read validator allows round-1-todos.md" +HOOK_INPUT='{"tool_name": "Read", "tool_input": {"file_path": "'$LOOP_DIR'/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-read-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Read validator allows round-1-todos.md" +else + fail "Read validator round-1-todos.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 16: Read validator allows round-0-summary.md (historical) +echo "Test 16: Read validator allows round-0-summary.md (historical)" +HOOK_INPUT='{"tool_name": "Read", "tool_input": {"file_path": "'$LOOP_DIR'/round-0-summary.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-read-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Read validator allows round-0-summary.md" +else + fail "Read validator round-0-summary.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 17: Read validator blocks round-3-todos.md +echo "Test 17: Read validator blocks round-3-todos.md" +HOOK_INPUT='{"tool_name": "Read", "tool_input": {"file_path": "'$LOOP_DIR'/round-3-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-read-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Read validator blocks round-3-todos.md" +else + fail "Read validator round-3-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 18: Read validator blocks round-3-summary.md (not in allowlist) +echo "Test 18: Read validator blocks round-3-summary.md" +HOOK_INPUT='{"tool_name": "Read", "tool_input": {"file_path": "'$LOOP_DIR'/round-3-summary.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-read-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "round"; then + pass "Read validator blocks round-3-summary.md" +else + fail "Read validator round-3-summary.md" "exit 2 with round error" "exit $EXIT_CODE, output: $RESULT" +fi + +echo "" +echo "=== Test: Bash Validator Allowlist (Path-Restricted) ===" +echo "" + +# Test 19: Bash validator allows round-1-todos.md in active loop dir path +echo "Test 19: Bash validator allows round-1-todos.md in active loop dir" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > '$LOOP_DIR'/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Bash validator allows round-1-todos.md in active loop dir" +else + fail "Bash validator round-1-todos.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 20: Bash validator allows round-2-todos.md in active loop dir path +echo "Test 20: Bash validator allows round-2-todos.md in active loop dir" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "cat data | tee '$LOOP_DIR'/round-2-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 0 ]]; then + pass "Bash validator allows round-2-todos.md in active loop dir" +else + fail "Bash validator round-2-todos.md" "exit 0" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 21: Bash validator blocks round-1-todos.md in wrong directory +echo "Test 21: Bash validator blocks round-1-todos.md in wrong directory" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > /tmp/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Bash validator blocks round-1-todos.md in wrong directory" +else + fail "Bash validator wrong dir round-1-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 22: Bash validator blocks round-3-todos.md (not in allowlist) +echo "Test 22: Bash validator blocks round-3-todos.md" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > '$LOOP_DIR'/round-3-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Bash validator blocks round-3-todos.md" +else + fail "Bash validator round-3-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 23: Bash validator blocks generic round-1-todos.md without full path +echo "Test 23: Bash validator blocks generic round-1-todos.md without full path" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Bash validator blocks generic round-1-todos.md" +else + fail "Bash validator generic round-1-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +# Test 24: Bash validator blocks round-1-todos.md in old loop directory +echo "Test 24: Bash validator blocks round-1-todos.md in old loop directory" +OLD_LOOP="$TEST_DIR/.humanize/rlcr/2023-01-01_00-00-00" +mkdir -p "$OLD_LOOP" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > '$OLD_LOOP'/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Bash validator blocks round-1-todos.md in old loop directory" +else + fail "Bash validator old loop round-1-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + +echo "" +echo "=========================================" +echo "Test Results" +echo "=========================================" +echo -e "Passed: ${GREEN}$TESTS_PASSED${NC}" +echo -e "Failed: ${RED}$TESTS_FAILED${NC}" +echo "" + +exit $TESTS_FAILED From 1fea22e96345f1005992936983b85317a156e3d5 Mon Sep 17 00:00:00 2001 From: Sihao Liu Date: Fri, 16 Jan 2026 02:11:53 -0800 Subject: [PATCH 17/17] Fix: Fail-closed bash allowlist using full path match - Changed bash validator to use full ACTIVE_LOOP_DIR path instead of basename-only check to prevent same-basename bypass from different roots - Added regex escaping for special characters in paths - Added Test 25 for same-basename different-root security scenario - Code-simplifier: consolidated comments, fixed regex escape pattern --- hooks/loop-bash-validator.sh | 6 ++++-- tests/test-allowlist-validators.sh | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/hooks/loop-bash-validator.sh b/hooks/loop-bash-validator.sh index 39702c9a..f6c0163b 100755 --- a/hooks/loop-bash-validator.sh +++ b/hooks/loop-bash-validator.sh @@ -134,8 +134,10 @@ fi # ======================================== if command_modifies_file "$COMMAND_LOWER" "round-[0-9]+-todos\.md"; then - ACTIVE_LOOP_DIRNAME=$(basename "$ACTIVE_LOOP_DIR") - if ! echo "$COMMAND_LOWER" | grep -qE "\.humanize/rlcr/${ACTIVE_LOOP_DIRNAME}/round-[12]-todos\.md"; then + # Require full path to active loop dir to prevent same-basename bypass from different roots + ACTIVE_LOOP_DIR_LOWER=$(to_lower "$ACTIVE_LOOP_DIR") + ACTIVE_LOOP_DIR_ESCAPED=$(echo "$ACTIVE_LOOP_DIR_LOWER" | sed 's/[\\.*^$[(){}+?|]/\\&/g') + if ! echo "$COMMAND_LOWER" | grep -qE "${ACTIVE_LOOP_DIR_ESCAPED}/round-[12]-todos\.md"; then todos_blocked_message "Bash" >&2 exit 2 fi diff --git a/tests/test-allowlist-validators.sh b/tests/test-allowlist-validators.sh index 453631c6..e6fc2a93 100755 --- a/tests/test-allowlist-validators.sh +++ b/tests/test-allowlist-validators.sh @@ -365,6 +365,21 @@ else fail "Bash validator old loop round-1-todos.md" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" fi +# Test 25: Bash validator blocks same-basename different-root (security test) +echo "Test 25: Bash validator blocks same-basename different-root" +ACTIVE_LOOP_BASENAME=$(basename "$LOOP_DIR") +DIFFERENT_ROOT="/tmp/.humanize/rlcr/${ACTIVE_LOOP_BASENAME}" +HOOK_INPUT='{"tool_name": "Bash", "tool_input": {"command": "echo test > '$DIFFERENT_ROOT'/round-1-todos.md"}}' +set +e +RESULT=$(echo "$HOOK_INPUT" | "$PROJECT_ROOT/hooks/loop-bash-validator.sh" 2>&1) +EXIT_CODE=$? +set -e +if [[ $EXIT_CODE -eq 2 ]] && echo "$RESULT" | grep -qi "todos"; then + pass "Bash validator blocks same-basename different-root" +else + fail "Bash validator same-basename different-root" "exit 2 with todos error" "exit $EXIT_CODE, output: $RESULT" +fi + echo "" echo "=========================================" echo "Test Results"