diff --git a/lib/response_analyzer.sh b/lib/response_analyzer.sh index 1e8c14cf..a8f3b507 100644 --- a/lib/response_analyzer.sh +++ b/lib/response_analyzer.sh @@ -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() { @@ -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: — 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 @@ -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" \ @@ -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, @@ -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") @@ -565,7 +627,9 @@ 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="" @@ -573,10 +637,13 @@ analyze_response() { 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=$( { @@ -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 @@ -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=$( { @@ -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 diff --git a/tests/unit/test_exit_detection.bats b/tests/unit/test_exit_detection.bats index 90ca60d9..0b082108 100644 --- a/tests/unit/test_exit_detection.bats +++ b/tests/unit/test_exit_detection.bats @@ -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" {