From 87206911356e85206cd7933c01d71f6f44bb6d7f Mon Sep 17 00:00:00 2001 From: Stefano Fiorini Date: Thu, 5 Mar 2026 22:54:50 -0600 Subject: [PATCH] feat(reviewer-runtime): add shared review supervisor --- docs/CREATE-PLAN.md | 19 ++ docs/IMPLEMENT-PLAN.md | 19 ++ skills/reviewer-runtime/run-review.sh | 303 ++++++++++++++++++++ skills/reviewer-runtime/tests/smoke-test.sh | 237 +++++++++++++++ 4 files changed, 578 insertions(+) create mode 100755 skills/reviewer-runtime/run-review.sh create mode 100644 skills/reviewer-runtime/tests/smoke-test.sh diff --git a/docs/CREATE-PLAN.md b/docs/CREATE-PLAN.md index b08a841..817cfd7 100644 --- a/docs/CREATE-PLAN.md +++ b/docs/CREATE-PLAN.md @@ -13,6 +13,11 @@ Create structured implementation plans with milestone and story tracking, and op - For Codex, native skill discovery must be configured: - `~/.agents/skills/superpowers -> ~/.codex/superpowers/skills` - For Cursor, skills must be installed under `.cursor/skills/` (repo-local) or `~/.cursor/skills/` (global) +- Shared reviewer runtime must be installed beside agent skills when using reviewer CLIs: + - Codex: `~/.codex/skills/reviewer-runtime/run-review.sh` + - Claude Code: `~/.claude/skills/reviewer-runtime/run-review.sh` + - OpenCode: `~/.config/opencode/skills/reviewer-runtime/run-review.sh` + - Cursor: `.cursor/skills/reviewer-runtime/run-review.sh` or `~/.cursor/skills/reviewer-runtime/run-review.sh` If dependencies are missing, stop and return: @@ -39,6 +44,8 @@ The reviewer CLI is independent of which agent is running the planning — e.g., ```bash mkdir -p ~/.codex/skills/create-plan cp -R skills/create-plan/codex/* ~/.codex/skills/create-plan/ +mkdir -p ~/.codex/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.codex/skills/reviewer-runtime/ ``` ### Claude Code @@ -46,6 +53,8 @@ cp -R skills/create-plan/codex/* ~/.codex/skills/create-plan/ ```bash mkdir -p ~/.claude/skills/create-plan cp -R skills/create-plan/claude-code/* ~/.claude/skills/create-plan/ +mkdir -p ~/.claude/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.claude/skills/reviewer-runtime/ ``` ### OpenCode @@ -53,6 +62,8 @@ cp -R skills/create-plan/claude-code/* ~/.claude/skills/create-plan/ ```bash mkdir -p ~/.config/opencode/skills/create-plan cp -R skills/create-plan/opencode/* ~/.config/opencode/skills/create-plan/ +mkdir -p ~/.config/opencode/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.config/opencode/skills/reviewer-runtime/ ``` ### Cursor @@ -62,6 +73,8 @@ Copy into the repo-local `.cursor/skills/` directory (where the Cursor Agent CLI ```bash mkdir -p .cursor/skills/create-plan cp -R skills/create-plan/cursor/* .cursor/skills/create-plan/ +mkdir -p .cursor/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* .cursor/skills/reviewer-runtime/ ``` Or install globally (loaded via `~/.cursor/skills/`): @@ -69,6 +82,8 @@ Or install globally (loaded via `~/.cursor/skills/`): ```bash mkdir -p ~/.cursor/skills/create-plan cp -R skills/create-plan/cursor/* ~/.cursor/skills/create-plan/ +mkdir -p ~/.cursor/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.cursor/skills/reviewer-runtime/ ``` ## Verify Installation @@ -78,6 +93,10 @@ test -f ~/.codex/skills/create-plan/SKILL.md || true test -f ~/.claude/skills/create-plan/SKILL.md || true test -f ~/.config/opencode/skills/create-plan/SKILL.md || true test -f .cursor/skills/create-plan/SKILL.md || test -f ~/.cursor/skills/create-plan/SKILL.md || true +test -x ~/.codex/skills/reviewer-runtime/run-review.sh || true +test -x ~/.claude/skills/reviewer-runtime/run-review.sh || true +test -x ~/.config/opencode/skills/reviewer-runtime/run-review.sh || true +test -x .cursor/skills/reviewer-runtime/run-review.sh || test -x ~/.cursor/skills/reviewer-runtime/run-review.sh || true ``` Verify Superpowers dependencies exist in your agent skills root: diff --git a/docs/IMPLEMENT-PLAN.md b/docs/IMPLEMENT-PLAN.md index d27b049..2a0cb31 100644 --- a/docs/IMPLEMENT-PLAN.md +++ b/docs/IMPLEMENT-PLAN.md @@ -20,6 +20,11 @@ Execute an existing plan (created by `create-plan`) in an isolated git worktree, - For Codex, native skill discovery must be configured: - `~/.agents/skills/superpowers -> ~/.codex/superpowers/skills` - For Cursor, skills must be installed under `.cursor/skills/` (repo-local) or `~/.cursor/skills/` (global) +- Shared reviewer runtime must be installed beside agent skills when using reviewer CLIs: + - Codex: `~/.codex/skills/reviewer-runtime/run-review.sh` + - Claude Code: `~/.claude/skills/reviewer-runtime/run-review.sh` + - OpenCode: `~/.config/opencode/skills/reviewer-runtime/run-review.sh` + - Cursor: `.cursor/skills/reviewer-runtime/run-review.sh` or `~/.cursor/skills/reviewer-runtime/run-review.sh` If dependencies are missing, stop and return: @@ -46,6 +51,8 @@ The reviewer CLI is independent of which agent is running the implementation — ```bash mkdir -p ~/.codex/skills/implement-plan cp -R skills/implement-plan/codex/* ~/.codex/skills/implement-plan/ +mkdir -p ~/.codex/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.codex/skills/reviewer-runtime/ ``` ### Claude Code @@ -53,6 +60,8 @@ cp -R skills/implement-plan/codex/* ~/.codex/skills/implement-plan/ ```bash mkdir -p ~/.claude/skills/implement-plan cp -R skills/implement-plan/claude-code/* ~/.claude/skills/implement-plan/ +mkdir -p ~/.claude/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.claude/skills/reviewer-runtime/ ``` ### OpenCode @@ -60,6 +69,8 @@ cp -R skills/implement-plan/claude-code/* ~/.claude/skills/implement-plan/ ```bash mkdir -p ~/.config/opencode/skills/implement-plan cp -R skills/implement-plan/opencode/* ~/.config/opencode/skills/implement-plan/ +mkdir -p ~/.config/opencode/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.config/opencode/skills/reviewer-runtime/ ``` ### Cursor @@ -69,6 +80,8 @@ Copy into the repo-local `.cursor/skills/` directory (where the Cursor Agent CLI ```bash mkdir -p .cursor/skills/implement-plan cp -R skills/implement-plan/cursor/* .cursor/skills/implement-plan/ +mkdir -p .cursor/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* .cursor/skills/reviewer-runtime/ ``` Or install globally (loaded via `~/.cursor/skills/`): @@ -76,6 +89,8 @@ Or install globally (loaded via `~/.cursor/skills/`): ```bash mkdir -p ~/.cursor/skills/implement-plan cp -R skills/implement-plan/cursor/* ~/.cursor/skills/implement-plan/ +mkdir -p ~/.cursor/skills/reviewer-runtime +cp -R skills/reviewer-runtime/* ~/.cursor/skills/reviewer-runtime/ ``` ## Verify Installation @@ -85,6 +100,10 @@ test -f ~/.codex/skills/implement-plan/SKILL.md || true test -f ~/.claude/skills/implement-plan/SKILL.md || true test -f ~/.config/opencode/skills/implement-plan/SKILL.md || true test -f .cursor/skills/implement-plan/SKILL.md || test -f ~/.cursor/skills/implement-plan/SKILL.md || true +test -x ~/.codex/skills/reviewer-runtime/run-review.sh || true +test -x ~/.claude/skills/reviewer-runtime/run-review.sh || true +test -x ~/.config/opencode/skills/reviewer-runtime/run-review.sh || true +test -x .cursor/skills/reviewer-runtime/run-review.sh || test -x ~/.cursor/skills/reviewer-runtime/run-review.sh || true ``` Verify Superpowers execution dependencies exist in your agent skills root: diff --git a/skills/reviewer-runtime/run-review.sh b/skills/reviewer-runtime/run-review.sh new file mode 100755 index 0000000..ef302e7 --- /dev/null +++ b/skills/reviewer-runtime/run-review.sh @@ -0,0 +1,303 @@ +#!/usr/bin/env bash +set -euo pipefail + +DEFAULT_POLL_SECONDS=10 +DEFAULT_SOFT_TIMEOUT_SECONDS=600 +DEFAULT_STALL_WARNING_SECONDS=300 +DEFAULT_HARD_TIMEOUT_SECONDS=1800 + +COMMAND_FILE="" +STDOUT_FILE="" +STDERR_FILE="" +STATUS_FILE="" +POLL_SECONDS=$DEFAULT_POLL_SECONDS +SOFT_TIMEOUT_SECONDS=$DEFAULT_SOFT_TIMEOUT_SECONDS +STALL_WARNING_SECONDS=$DEFAULT_STALL_WARNING_SECONDS +HARD_TIMEOUT_SECONDS=$DEFAULT_HARD_TIMEOUT_SECONDS + +CHILD_PID="" +USE_GROUP_KILL=0 +INTERRUPTED=0 + +usage() { + cat <<'EOF' +Usage: + run-review.sh \ + --command-file \ + --stdout-file \ + --stderr-file \ + --status-file \ + [--poll-seconds ] \ + [--soft-timeout-seconds ] \ + [--stall-warning-seconds ] \ + [--hard-timeout-seconds ] +EOF +} + +fail_usage() { + echo "Error: $*" >&2 + usage >&2 + exit 2 +} + +require_integer() { + local name=$1 + local value=$2 + [[ "$value" =~ ^[0-9]+$ ]] || fail_usage "$name must be an integer" +} + +escape_note() { + local note=$1 + note=${note//$'\n'/ } + note=${note//\"/\'} + printf '%s' "$note" +} + +iso_timestamp() { + date -u +"%Y-%m-%dT%H:%M:%SZ" +} + +elapsed_seconds() { + local now + now=$(date +%s) + printf '%s' $((now - START_TIME)) +} + +file_bytes() { + local path=$1 + if [[ -f "$path" ]]; then + wc -c <"$path" | tr -d '[:space:]' + else + printf '0' + fi +} + +append_status() { + local level=$1 + local state=$2 + local note=$3 + local elapsed pid stdout_bytes stderr_bytes line + + elapsed=$(elapsed_seconds) + pid=${CHILD_PID:-0} + stdout_bytes=$(file_bytes "$STDOUT_FILE") + stderr_bytes=$(file_bytes "$STDERR_FILE") + line="ts=$(iso_timestamp) level=$level state=$state elapsed_s=$elapsed pid=$pid stdout_bytes=$stdout_bytes stderr_bytes=$stderr_bytes note=\"$(escape_note "$note")\"" + + printf '%s\n' "$line" | tee -a "$STATUS_FILE" +} + +ensure_parent_dir() { + local path=$1 + mkdir -p "$(dirname "$path")" +} + +kill_child_process_group() { + if [[ -z "$CHILD_PID" ]]; then + return 0 + fi + + if ! kill -0 "$CHILD_PID" 2>/dev/null; then + return 0 + fi + + if [[ "$USE_GROUP_KILL" -eq 1 ]]; then + kill -TERM -- "-$CHILD_PID" 2>/dev/null || kill -TERM "$CHILD_PID" 2>/dev/null || true + else + kill -TERM "$CHILD_PID" 2>/dev/null || true + fi + + sleep 1 + + if kill -0 "$CHILD_PID" 2>/dev/null; then + if [[ "$USE_GROUP_KILL" -eq 1 ]]; then + kill -KILL -- "-$CHILD_PID" 2>/dev/null || kill -KILL "$CHILD_PID" 2>/dev/null || true + else + kill -KILL "$CHILD_PID" 2>/dev/null || true + fi + fi +} + +handle_signal() { + local signal_name=$1 + INTERRUPTED=1 + append_status error failed "received SIG${signal_name}; terminating reviewer child" + kill_child_process_group + exit 130 +} + +parse_args() { + while [[ $# -gt 0 ]]; do + case "$1" in + --command-file) + COMMAND_FILE=${2:-} + shift 2 + ;; + --stdout-file) + STDOUT_FILE=${2:-} + shift 2 + ;; + --stderr-file) + STDERR_FILE=${2:-} + shift 2 + ;; + --status-file) + STATUS_FILE=${2:-} + shift 2 + ;; + --poll-seconds) + POLL_SECONDS=${2:-} + shift 2 + ;; + --soft-timeout-seconds) + SOFT_TIMEOUT_SECONDS=${2:-} + shift 2 + ;; + --stall-warning-seconds) + STALL_WARNING_SECONDS=${2:-} + shift 2 + ;; + --hard-timeout-seconds) + HARD_TIMEOUT_SECONDS=${2:-} + shift 2 + ;; + --help|-h) + usage + exit 0 + ;; + *) + fail_usage "unknown argument: $1" + ;; + esac + done + + [[ -n "$COMMAND_FILE" ]] || fail_usage "--command-file is required" + [[ -n "$STDOUT_FILE" ]] || fail_usage "--stdout-file is required" + [[ -n "$STDERR_FILE" ]] || fail_usage "--stderr-file is required" + [[ -n "$STATUS_FILE" ]] || fail_usage "--status-file is required" + + require_integer "poll-seconds" "$POLL_SECONDS" + require_integer "soft-timeout-seconds" "$SOFT_TIMEOUT_SECONDS" + require_integer "stall-warning-seconds" "$STALL_WARNING_SECONDS" + require_integer "hard-timeout-seconds" "$HARD_TIMEOUT_SECONDS" + + [[ "$POLL_SECONDS" -gt 0 ]] || fail_usage "poll-seconds must be > 0" + [[ "$SOFT_TIMEOUT_SECONDS" -gt 0 ]] || fail_usage "soft-timeout-seconds must be > 0" + [[ "$STALL_WARNING_SECONDS" -gt 0 ]] || fail_usage "stall-warning-seconds must be > 0" + [[ "$HARD_TIMEOUT_SECONDS" -gt 0 ]] || fail_usage "hard-timeout-seconds must be > 0" + [[ "$SOFT_TIMEOUT_SECONDS" -le "$HARD_TIMEOUT_SECONDS" ]] || fail_usage "soft-timeout-seconds must be <= hard-timeout-seconds" + [[ "$STALL_WARNING_SECONDS" -le "$HARD_TIMEOUT_SECONDS" ]] || fail_usage "stall-warning-seconds must be <= hard-timeout-seconds" + + [[ -r "$COMMAND_FILE" ]] || fail_usage "command file is not readable: $COMMAND_FILE" +} + +launch_child() { + if command -v setsid >/dev/null 2>&1; then + setsid bash "$COMMAND_FILE" >"$STDOUT_FILE" 2>"$STDERR_FILE" & + USE_GROUP_KILL=1 + else + bash "$COMMAND_FILE" >"$STDOUT_FILE" 2>"$STDERR_FILE" & + USE_GROUP_KILL=0 + fi + CHILD_PID=$! +} + +main() { + parse_args "$@" + + ensure_parent_dir "$STDOUT_FILE" + ensure_parent_dir "$STDERR_FILE" + ensure_parent_dir "$STATUS_FILE" + : >"$STDOUT_FILE" + : >"$STDERR_FILE" + : >"$STATUS_FILE" + + START_TIME=$(date +%s) + export START_TIME + + trap 'handle_signal INT' INT + trap 'handle_signal TERM' TERM + trap 'if [[ "$INTERRUPTED" -eq 0 ]]; then kill_child_process_group; fi' EXIT + + launch_child + append_status info running-silent "reviewer child launched" + + local last_stdout_bytes=0 + local last_stderr_bytes=0 + local last_output_change_time=$START_TIME + local soft_timeout_logged=0 + local stall_warning_logged=0 + + while kill -0 "$CHILD_PID" 2>/dev/null; do + sleep "$POLL_SECONDS" + + local now elapsed stdout_bytes stderr_bytes note state level + now=$(date +%s) + elapsed=$((now - START_TIME)) + stdout_bytes=$(file_bytes "$STDOUT_FILE") + stderr_bytes=$(file_bytes "$STDERR_FILE") + + if [[ "$stdout_bytes" -ne "$last_stdout_bytes" || "$stderr_bytes" -ne "$last_stderr_bytes" ]]; then + last_output_change_time=$now + stall_warning_logged=0 + state=running-active + level=info + note="reviewer output changed" + else + local silent_for + silent_for=$((now - last_output_change_time)) + if [[ "$silent_for" -ge "$STALL_WARNING_SECONDS" ]]; then + state=stall-warning + level=warn + note="no output growth for ${silent_for}s; process still alive" + stall_warning_logged=1 + else + state=running-silent + level=info + note="reviewer process alive; waiting for output" + fi + fi + + if [[ "$elapsed" -ge "$SOFT_TIMEOUT_SECONDS" && "$soft_timeout_logged" -eq 0 ]]; then + note="$note; soft timeout reached, continuing while reviewer is alive" + soft_timeout_logged=1 + fi + + append_status "$level" "$state" "$note" + last_stdout_bytes=$stdout_bytes + last_stderr_bytes=$stderr_bytes + + if [[ "$elapsed" -ge "$HARD_TIMEOUT_SECONDS" ]]; then + append_status error needs-operator-decision "hard timeout reached; terminating reviewer child for operator intervention" + kill_child_process_group + trap - EXIT + exit 12 + fi + done + + local child_exit_code=0 + set +e + wait "$CHILD_PID" + child_exit_code=$? + set -e + trap - EXIT + + local final_stdout_bytes final_stderr_bytes + final_stdout_bytes=$(file_bytes "$STDOUT_FILE") + final_stderr_bytes=$(file_bytes "$STDERR_FILE") + + if [[ "$child_exit_code" -eq 0 ]]; then + if [[ "$final_stdout_bytes" -gt 0 ]]; then + append_status info completed "reviewer completed successfully" + exit 0 + fi + + append_status error completed-empty-output "reviewer exited successfully with empty stdout" + exit 10 + fi + + append_status error failed "reviewer exited with code $child_exit_code" + exit "$child_exit_code" +} + +main "$@" diff --git a/skills/reviewer-runtime/tests/smoke-test.sh b/skills/reviewer-runtime/tests/smoke-test.sh new file mode 100644 index 0000000..1f09014 --- /dev/null +++ b/skills/reviewer-runtime/tests/smoke-test.sh @@ -0,0 +1,237 @@ +#!/usr/bin/env bash +set -euo pipefail + +SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +HELPER_PATH=$(cd "$SCRIPT_DIR/.." && pwd)/run-review.sh + +fail() { + echo "FAIL: $*" >&2 + exit 1 +} + +assert_file_contains() { + local file=$1 + local pattern=$2 + if ! rg -q --fixed-strings "$pattern" "$file"; then + echo "Expected pattern not found: $pattern" >&2 + echo "--- $file ---" >&2 + sed -n '1,200p' "$file" >&2 || true + fail "missing pattern in $file" + fi +} + +assert_exit_code() { + local actual=$1 + local expected=$2 + if [[ "$actual" -ne "$expected" ]]; then + fail "expected exit code $expected, got $actual" + fi +} + +assert_nonzero_exit() { + local actual=$1 + if [[ "$actual" -eq 0 ]]; then + fail "expected non-zero exit code" + fi +} + +make_command() { + local file=$1 + local body=$2 + cat >"$file" <&2 +exit 7 +' + + if run_helper "$command_file" "$stdout_file" "$stderr_file" "$status_file" \ + --poll-seconds 1 \ + --soft-timeout-seconds 5 \ + --stall-warning-seconds 3 \ + --hard-timeout-seconds 10; then + local exit_code=0 + else + local exit_code=$? + fi + + assert_nonzero_exit "$exit_code" + assert_file_contains "$stderr_file" "boom" + assert_file_contains "$status_file" "state=failed" +} + +test_empty_output_is_terminal() { + local dir=$1 + local command_file=$dir/empty-output.sh + local stdout_file=$dir/empty-output.stdout + local stderr_file=$dir/empty-output.stderr + local status_file=$dir/empty-output.status + + make_command "$command_file" ' +sleep 1 +exit 0 +' + + if run_helper "$command_file" "$stdout_file" "$stderr_file" "$status_file" \ + --poll-seconds 1 \ + --soft-timeout-seconds 5 \ + --stall-warning-seconds 3 \ + --hard-timeout-seconds 10; then + local exit_code=0 + else + local exit_code=$? + fi + + assert_nonzero_exit "$exit_code" + assert_file_contains "$status_file" "state=completed-empty-output" +} + +test_signal_cleanup() { + local dir=$1 + local command_file=$dir/signal-child.sh + local stdout_file=$dir/signal-child.stdout + local stderr_file=$dir/signal-child.stderr + local status_file=$dir/signal-child.status + local child_pid_file=$dir/child.pid + + make_command "$command_file" " +printf '%s\n' \"\$\$\" > \"$child_pid_file\" +sleep 30 +" + + set +e + "$HELPER_PATH" \ + --command-file "$command_file" \ + --stdout-file "$stdout_file" \ + --stderr-file "$stderr_file" \ + --status-file "$status_file" \ + --poll-seconds 1 \ + --soft-timeout-seconds 5 \ + --stall-warning-seconds 2 \ + --hard-timeout-seconds 10 & + local helper_pid=$! + set -e + + sleep 2 + kill -TERM "$helper_pid" + + set +e + wait "$helper_pid" + local exit_code=$? + set -e + + assert_nonzero_exit "$exit_code" + [[ -f "$child_pid_file" ]] || fail "child pid file was not written" + + local child_pid + child_pid=$(cat "$child_pid_file") + sleep 1 + if kill -0 "$child_pid" 2>/dev/null; then + fail "child process is still alive after helper termination" + fi +} + +main() { + [[ -x "$HELPER_PATH" ]] || fail "helper is not executable: $HELPER_PATH" + + local tmp_dir + tmp_dir=$(mktemp -d) + trap "rm -rf '$tmp_dir'" EXIT + + test_delayed_success "$tmp_dir" + test_soft_timeout_continues "$tmp_dir" + test_nonzero_failure "$tmp_dir" + test_empty_output_is_terminal "$tmp_dir" + test_signal_cleanup "$tmp_dir" + + echo "PASS: reviewer runtime smoke tests" +} + +main "$@"