Skip to content

fix(response_analyzer): progress detection in non-git multi-repo workspaces #342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DrumRobot wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(response_analyzer): progress detection in non-git multi-repo workspaces #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 84 additions & 12 deletions lib/response_analyzer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,26 @@ _base_command_in_allowed_tools() {
return 1
}

# Resolve where git-based progress detection should run (Issue #340).
# Prefers the loop CWD's repository; falls back to $RALPH_DIR's own repository
# when the CWD is not inside a git work tree (multi-repo workspace root whose
# top-level folder is not itself a git repo — tracker updates in .ralph/ are
# then the progress proxy).
# Sets GIT_PROGRESS_DIR ("." for the CWD repo, "$RALPH_DIR" for the fallback).
# Returns 1 when no repository is available in either location.
_resolve_git_progress_dir() {
GIT_PROGRESS_DIR=""
if git rev-parse --git-dir >/dev/null 2>&1; then
GIT_PROGRESS_DIR="."
return 0
fi
if [[ -n "${RALPH_DIR:-}" ]] && git -C "$RALPH_DIR" rev-parse --git-dir >/dev/null 2>&1; then
GIT_PROGRESS_DIR="$RALPH_DIR"
return 0
fi
return 1
}

# Detect output format (json or text)
# Returns: "json" if valid JSON, "text" otherwise
detect_output_format() {
Expand Down Expand Up @@ -317,6 +337,36 @@ parse_json_response() {
# Files modified: from flat format OR from metadata.files_changed
local files_modified=$(jq -r '.metadata.files_changed // .files_modified // 0' "$output_file" 2>/dev/null)

# Issue #340: explicit progress self-report from the RALPH_STATUS block.
# When the loop CWD is not a git repository (multi-repo workspace root),
# git-based progress detection is blind, so the agent can self-report via
# the same block already used for EXIT_SIGNAL:
# FILES_MODIFIED: <N> — file-change count (e.g. commits made via git -C)
# PROGRESS: true — non-file work (e.g. posting a PR review)
# Same explicit-intent semantics as the EXIT_SIGNAL extraction above; both
# lines are anchored at start-of-line so prose mentions do not trigger them.
local self_reported_progress="false"
if [[ "$has_result_field" == "true" ]]; then
local rs_result_text=$(jq -r '.result // ""' "$output_file" 2>/dev/null)
if [[ -n "$rs_result_text" ]] && echo "$rs_result_text" | grep -qE -- "^[[:space:]]*(---RALPH_STATUS---|RALPH_STATUS:)"; then
local embedded_files_modified
embedded_files_modified=$(echo "$rs_result_text" | grep -E "^[[:space:]]*FILES_MODIFIED:" | head -1 | cut -d: -f2 | xargs)
if [[ "$embedded_files_modified" =~ ^[0-9]+$ ]]; then
# Structured metadata (when present and numeric) stays authoritative;
# the self-report only fills the gap, it never lowers the count
[[ "$files_modified" =~ ^[0-9]+$ ]] || files_modified=0
if (( embedded_files_modified > files_modified )); then
files_modified=$embedded_files_modified
fi
fi
local embedded_progress
embedded_progress=$(echo "$rs_result_text" | grep -E "^[[:space:]]*PROGRESS:" | head -1 | cut -d: -f2 | xargs)
if [[ "$embedded_progress" == "true" ]]; then
self_reported_progress="true"
fi
fi
fi

# Error count: from flat format OR derived from metadata.has_errors
# Note: When only has_errors=true is present (without explicit error_count),
# we set error_count=1 as a minimum. This is defensive programming since
Expand Down Expand Up @@ -453,6 +503,7 @@ parse_json_response() {
--argjson is_stuck "$is_stuck" \
--argjson has_completion_signal "$has_completion_signal" \
--argjson files_modified "$files_modified" \
--argjson self_reported_progress "$self_reported_progress" \
--argjson error_count "$error_count" \
--arg summary "$summary" \
--argjson loop_number "$loop_number" \
Expand All @@ -470,6 +521,7 @@ parse_json_response() {
is_stuck: $is_stuck,
has_completion_signal: $has_completion_signal,
files_modified: $files_modified,
self_reported_progress: $self_reported_progress,
error_count: $error_count,
summary: $summary,
loop_number: $loop_number,
Expand Down Expand Up @@ -535,6 +587,16 @@ analyze_response() {
local json_confidence=$(jq -r '.confidence' $RALPH_DIR/.json_parse_result 2>/dev/null || echo "0")
local session_id=$(jq -r '.session_id' $RALPH_DIR/.json_parse_result 2>/dev/null || echo "")

# Issue #340: structured or self-reported file changes count as progress
# even when git-based detection below is unavailable (non-git CWD)
local self_reported_progress=$(jq -r '.self_reported_progress // false' $RALPH_DIR/.json_parse_result 2>/dev/null || echo "false")
if [[ "$files_modified" =~ ^[0-9]+$ ]] && (( files_modified > 0 )); then
has_progress=true
fi
if [[ "$self_reported_progress" == "true" ]]; then
has_progress=true
fi

# Extract permission denial fields (Issue #101)
local has_permission_denials=$(jq -r '.has_permission_denials' $RALPH_DIR/.json_parse_result 2>/dev/null || echo "false")
local permission_denial_count=$(jq -r '.permission_denial_count' $RALPH_DIR/.json_parse_result 2>/dev/null || echo "0")
Expand Down Expand Up @@ -565,18 +627,23 @@ analyze_response() {

# Check for file changes via git (supplements JSON data)
# Fix #141: Detect both uncommitted changes AND committed changes
if command -v git &>/dev/null && git rev-parse --git-dir >/dev/null 2>&1; then
# Issue #340: fall back to $RALPH_DIR's own repository when the CWD
# is not a git work tree (multi-repo workspace root)
if command -v git &>/dev/null && _resolve_git_progress_dir; then
local git_files=0
local loop_start_sha=""
local current_sha=""

if [[ -f "$RALPH_DIR/.loop_start_sha" ]]; then
loop_start_sha=$(cat "$RALPH_DIR/.loop_start_sha" 2>/dev/null || echo "")
fi
current_sha=$(git rev-parse HEAD 2>/dev/null || echo "")
current_sha=$(git -C "$GIT_PROGRESS_DIR" rev-parse HEAD 2>/dev/null || echo "")

# Check if commits were made (HEAD changed)
if [[ -n "$loop_start_sha" && -n "$current_sha" && "$loop_start_sha" != "$current_sha" ]]; then
# Check if commits were made (HEAD changed).
# .loop_start_sha records the CWD repo's HEAD, so the commit-range
# comparison only applies when detection runs in the CWD repo —
# in tracker-repo fallback mode only uncommitted changes count.
if [[ "$GIT_PROGRESS_DIR" == "." && -n "$loop_start_sha" && -n "$current_sha" && "$loop_start_sha" != "$current_sha" ]]; then
# Commits were made - count union of committed files AND working tree changes
git_files=$(
{
Expand All @@ -589,8 +656,8 @@ analyze_response() {
# No commits - check for uncommitted changes (staged + unstaged)
git_files=$(
{
git diff --name-only 2>/dev/null # unstaged changes
git diff --name-only --cached 2>/dev/null # staged changes
git -C "$GIT_PROGRESS_DIR" diff --name-only 2>/dev/null # unstaged changes
git -C "$GIT_PROGRESS_DIR" diff --name-only --cached 2>/dev/null # staged changes
} | sort -u | wc -l
)
fi
Expand Down Expand Up @@ -760,17 +827,22 @@ analyze_response() {

# 6. Check for file changes (git integration)
# Fix #141: Detect both uncommitted changes AND committed changes
if command -v git &>/dev/null && git rev-parse --git-dir >/dev/null 2>&1; then
# Issue #340: fall back to $RALPH_DIR's own repository when the CWD
# is not a git work tree (multi-repo workspace root)
if command -v git &>/dev/null && _resolve_git_progress_dir; then
local loop_start_sha=""
local current_sha=""

if [[ -f "$RALPH_DIR/.loop_start_sha" ]]; then
loop_start_sha=$(cat "$RALPH_DIR/.loop_start_sha" 2>/dev/null || echo "")
fi
current_sha=$(git rev-parse HEAD 2>/dev/null || echo "")
current_sha=$(git -C "$GIT_PROGRESS_DIR" rev-parse HEAD 2>/dev/null || echo "")

# Check if commits were made (HEAD changed)
if [[ -n "$loop_start_sha" && -n "$current_sha" && "$loop_start_sha" != "$current_sha" ]]; then
# Check if commits were made (HEAD changed).
# .loop_start_sha records the CWD repo's HEAD, so the commit-range
# comparison only applies when detection runs in the CWD repo —
# in tracker-repo fallback mode only uncommitted changes count.
if [[ "$GIT_PROGRESS_DIR" == "." && -n "$loop_start_sha" && -n "$current_sha" && "$loop_start_sha" != "$current_sha" ]]; then
# Commits were made - count union of committed files AND working tree changes
files_modified=$(
{
Expand All @@ -783,8 +855,8 @@ analyze_response() {
# No commits - check for uncommitted changes (staged + unstaged)
files_modified=$(
{
git diff --name-only 2>/dev/null # unstaged changes
git diff --name-only --cached 2>/dev/null # staged changes
git -C "$GIT_PROGRESS_DIR" diff --name-only 2>/dev/null # unstaged changes
git -C "$GIT_PROGRESS_DIR" diff --name-only --cached 2>/dev/null # staged changes
} | sort -u | wc -l
)
fi
Expand Down
135 changes: 135 additions & 0 deletions tests/unit/test_exit_detection.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1556,6 +1556,141 @@ detect_progress_with_commits() {
assert_equal "$((score_decline - score_no_decline))" "10"
}

# =============================================================================
# PROGRESS DETECTION IN NON-GIT WORKSPACES (Issue #340)
# =============================================================================
# When the loop CWD is not a git repository (multi-repo workspace root),
# git-based progress detection is skipped entirely. These tests cover the two
# fallback signal paths: (1) the $RALPH_DIR tracker repository, and (2) an
# explicit agent self-report in the RALPH_STATUS block.

@test "analyze_response detects progress from RALPH_DIR repo when CWD is not a git repo (issue #340)" {
# Skip if git is not available (analyze_response uses git)
if ! command -v git &>/dev/null; then
skip "git not available"
fi

# CWD (TEST_TEMP_DIR) is intentionally NOT a git repository.
# The tracker directory is its own repository with a tracked fix_plan.md.
git -C "$RALPH_DIR" init --quiet
git -C "$RALPH_DIR" config user.email "test@test.com"
git -C "$RALPH_DIR" config user.name "Test"
echo "- [ ] item one" > "$RALPH_DIR/fix_plan.md"
git -C "$RALPH_DIR" add fix_plan.md
git -C "$RALPH_DIR" commit --quiet -m "baseline"

# Simulate loop work: the tracker file is updated during the loop
echo "- [x] item one" > "$RALPH_DIR/fix_plan.md"

source "${BATS_TEST_DIRNAME}/../../lib/response_analyzer.sh"
mkdir -p "$RALPH_DIR/logs"

local output_file="$RALPH_DIR/logs/claude_output_test.log"
echo "Consolidated the PR review and updated the tracker." > "$output_file"

run analyze_response "$output_file" 1

assert_success
local progress=$(jq -r '.analysis.has_progress' "$RALPH_DIR/.response_analysis")
local files=$(jq -r '.analysis.files_modified' "$RALPH_DIR/.response_analysis")
assert_equal "$progress" "true"
assert_equal "$files" "1"
}

@test "analyze_response does not crash when neither CWD nor RALPH_DIR is a git repo (issue #340)" {
# Skip if git is not available (analyze_response uses git)
if ! command -v git &>/dev/null; then
skip "git not available"
fi

# No git repository anywhere: CWD is plain, RALPH_DIR is plain
source "${BATS_TEST_DIRNAME}/../../lib/response_analyzer.sh"
mkdir -p "$RALPH_DIR/logs"

local output_file="$RALPH_DIR/logs/claude_output_test.log"
echo "Worked on documentation." > "$output_file"

run analyze_response "$output_file" 1

assert_success
[ -f "$RALPH_DIR/.response_analysis" ]
local progress=$(jq -r '.analysis.has_progress' "$RALPH_DIR/.response_analysis")
assert_equal "$progress" "false"
}

@test "analyze_response trusts FILES_MODIFIED self-report in RALPH_STATUS block (issue #340)" {
# Skip if git is not available (analyze_response uses git)
if ! command -v git &>/dev/null; then
skip "git not available"
fi

# Non-git CWD, no tracker repo: only the agent self-report is available
source "${BATS_TEST_DIRNAME}/../../lib/response_analyzer.sh"
mkdir -p "$RALPH_DIR/logs"

local output_file="$RALPH_DIR/logs/claude_output_test.json"
jq -n '{
result: "Committed fixes in a sub-repository via git -C.\n---RALPH_STATUS---\nSTATUS: IN_PROGRESS\nEXIT_SIGNAL: false\nFILES_MODIFIED: 3\n---END_RALPH_STATUS---",
sessionId: "test-session"
}' > "$output_file"

run analyze_response "$output_file" 1

assert_success
local progress=$(jq -r '.analysis.has_progress' "$RALPH_DIR/.response_analysis")
local files=$(jq -r '.analysis.files_modified' "$RALPH_DIR/.response_analysis")
assert_equal "$progress" "true"
assert_equal "$files" "3"
}

@test "analyze_response trusts PROGRESS: true self-report without FILES_MODIFIED (issue #340)" {
# Skip if git is not available (analyze_response uses git)
if ! command -v git &>/dev/null; then
skip "git not available"
fi

# Work with no local file changes at all (e.g. posting a PR review):
# the agent reports progress explicitly
source "${BATS_TEST_DIRNAME}/../../lib/response_analyzer.sh"
mkdir -p "$RALPH_DIR/logs"

local output_file="$RALPH_DIR/logs/claude_output_test.json"
jq -n '{
result: "Posted the consolidated review summary on the open PR.\n---RALPH_STATUS---\nSTATUS: IN_PROGRESS\nEXIT_SIGNAL: false\nPROGRESS: true\n---END_RALPH_STATUS---",
sessionId: "test-session"
}' > "$output_file"

run analyze_response "$output_file" 1

assert_success
local progress=$(jq -r '.analysis.has_progress' "$RALPH_DIR/.response_analysis")
local files=$(jq -r '.analysis.files_modified' "$RALPH_DIR/.response_analysis")
assert_equal "$progress" "true"
assert_equal "$files" "0"
}

@test "analyze_response keeps has_progress false on explicit FILES_MODIFIED: 0 (issue #340)" {
# Skip if git is not available (analyze_response uses git)
if ! command -v git &>/dev/null; then
skip "git not available"
fi

source "${BATS_TEST_DIRNAME}/../../lib/response_analyzer.sh"
mkdir -p "$RALPH_DIR/logs"

local output_file="$RALPH_DIR/logs/claude_output_test.json"
jq -n '{
result: "Re-scanned open PRs; nothing actionable this loop.\n---RALPH_STATUS---\nSTATUS: IN_PROGRESS\nEXIT_SIGNAL: false\nFILES_MODIFIED: 0\n---END_RALPH_STATUS---",
sessionId: "test-session"
}' > "$output_file"

run analyze_response "$output_file" 1

assert_success
local progress=$(jq -r '.analysis.has_progress' "$RALPH_DIR/.response_analysis")
assert_equal "$progress" "false"
}

# --- Stale Exit Signals Tests (Issue #194) ---

@test "startup resets stale exit signals before main loop" {
Expand Down