Repository navigation
fix(response_analyzer): progress detection in non-git multi-repo workspaces - #342
Conversation
…spaces (frankbria#340) When the loop CWD is not a git repository (multi-repo workspace root), the git rev-parse gate skipped file-change detection entirely, so has_progress stayed false on every loop regardless of real work, and the circuit breaker opened unconditionally after the no-progress threshold. Three complementary signal paths fix the class of false positives: 1. RALPH_DIR repo fallback: _resolve_git_progress_dir() prefers the CWD repository and falls back to $RALPH_DIR's own repository — tracker updates (fix_plan.md) become the progress proxy. The .loop_start_sha commit-range comparison only applies in CWD-repo mode since the recorded SHA belongs to the CWD repository. 2. RALPH_STATUS self-report: parse_json_response() extracts FILES_MODIFIED: <N> and PROGRESS: true from the RALPH_STATUS block in the .result text, with the same explicit-intent semantics (and the same start-of-line anchoring) as the existing EXIT_SIGNAL extraction. Structured metadata stays authoritative; the self-report never lowers the count. 3. JSON-path has_progress from files_modified: a structured or self-reported positive files_modified now sets has_progress directly instead of relying exclusively on git detection. 5 regression tests: tracker-repo fallback detects a modified fix_plan.md; no-repo-anywhere still completes with has_progress=false; FILES_MODIFIED self-report sets count + progress; PROGRESS: true works without a file count; explicit FILES_MODIFIED: 0 does not fabricate progress. Fixes frankbria#340 Contributed by @daegunjhy Signed-off-by: Hayoung Jeong <drumrobot43@gmail.com>
frankbria
left a comment
There was a problem hiding this comment.
Thanks for this — the $RALPH_DIR repo fallback (_resolve_git_progress_dir) is a clean fix for #340, and keeping the .loop_start_sha commit-range check CWD-only is the right call. One blocking issue with the self-report path, though.
Blocking: the FILES_MODIFIED self-report applies to every project, not just non-git workspaces
templates/PROMPT.md already instructs Claude to emit FILES_MODIFIED: <number> in every RALPH_STATUS block, so this extraction (lib/response_analyzer.sh:340) fires for essentially all Ralph projects. Combined with the new block at :590, any positive self-reported count sets has_progress=true — and the git detection below can only ever set has_progress, never clear it.
Concrete scenario in an ordinary git repo: Claude reports FILES_MODIFIED: 3 but changes nothing on disk → git finds 0 files → has_progress stays true → consecutive_no_progress never increments → the CB_NO_PROGRESS_THRESHOLD circuit breaker can't open. The no-progress breaker exists precisely to catch loops where the agent claims work it isn't doing, so letting the agent's claim satisfy it removes that guard for everyone.
Suggested fix: only honor FILES_MODIFIED / PROGRESS: true self-reports when git-based detection is unavailable (i.e. _resolve_git_progress_dir returns 1). When a repo is available, git stays authoritative. Ideally add a regression test: git repo + no changes + FILES_MODIFIED: 3 → has_progress=false.
The same reasoning applies to the structured files_modified > 0 → has_progress=true shortcut — worth gating the same way.
Verified
Merged onto current main locally: full unit + integration suite passes (1284/1284), no conflicts with main or the other open PRs.
…ted progress (frankbria#340) Runs the real loop with a mock claude in a workspace whose CWD is not a git work tree; Claude reports FILES_MODIFIED each loop. Fails on main (the breaker trips before the 4th call) and passes with this PR. Claude-Session: https://claude.ai/code/session_01SFomvXNxUJZ8q72SMEEmEN
…t only (frankbria#340) codex review of this PR (both verified with failing e2e tests first): - PROGRESS: true only set .analysis.has_progress, which record_loop_result never reads, so it never kept the breaker closed. It is now written as .analysis.self_reported_progress (both analysis writers) and honored by the circuit breaker. - The FILES_MODIFIED / PROGRESS self-report was only parsed from JSON output; text output mode now parses the same anchored lines. Maintainer decision: self-reports are trusted only where git can't see the work. Both parse sites are gated on the loop CWD not being a git work tree, so in a git repo a model claiming progress without real changes still trips the breaker (new e2e test). Claude-Session: https://claude.ai/code/session_01SFomvXNxUJZ8q72SMEEmEN
|
Thanks @DrumRobot. This is the fix we're taking for #340 (#366 will be closed in its favor). I merged
Cross-family review: codex (fallback; opencode/GLM timed out)The patch adds progress self-reporting, but one of its advertised signals is not consumed by the circuit breaker, and the extraction is missing for the supported text output path. These cases can still incorrectly trip the no-progress circuit breaker. Full review comments:
Triage: both P2s are fixed in b802987, with tests. Note that the GLM review check can't run on fork PRs (no secrets), which is why this review was run locally. |
PROMPT template + ralph-enable generated prompt: when the project root isn't a git repo, report FILES_MODIFIED and use PROGRESS: true for non-file progress (ignored in git repos). CLAUDE.md circuit-breaker section describes the fallback and the non-git-only trust boundary. Claude-Session: https://claude.ai/code/session_01SFomvXNxUJZ8q72SMEEmEN
Summary
Fixes #340 — in a multi-repo workspace whose root folder is not a git repository,
analyze_responsecould never see progress: thegit rev-parse --git-dirgate failed every loop, file-change detection was skipped entirely, and the Claude CLI result JSON carries nofiles_modifiedfield. Every loop counted as no-progress and the circuit breaker opened unconditionally after the threshold.This PR adds three complementary signal paths (any one of them ends the false-positive class):
$RALPH_DIRrepo fallback — new_resolve_git_progress_dir()helper prefers the CWD repository and falls back to the tracker directory's own repository. Tracker updates (fix_plan.md) become the progress proxy. The.loop_start_shacommit-range comparison stays CWD-repo-only, since the recorded SHA belongs to the CWD repository; fallback mode counts uncommitted changes only.RALPH_STATUSself-report —parse_json_response()now extractsFILES_MODIFIED: <N>andPROGRESS: truefrom theRALPH_STATUSblock embedded in the.resulttext, with the same explicit-intent semantics and the same start-of-line anchoring as the existingEXIT_SIGNALextraction (prose mentions do not trigger). Structured metadata stays authoritative — the self-report fills the gap, never lowers the count.has_progressfromfiles_modified— a structured or self-reported positivefiles_modifiedsetshas_progressdirectly, instead of relying exclusively on git detection.Scope note: the self-report extraction is implemented for the JSON path (
.resultfield) only, matching the Claude CLI output format where the bug was observed. Text-mode outputs already benefit from path 1.Test plan
tests/unit/test_exit_detection.bats(issue [P2.1] Circuit breaker false positive: progress detection is blind when the loop CWD is not a git repository (multi-repo workspace) #340 section):fix_plan.mdwhen the CWD is not a repo (fails onmain, passes here)has_progress=false(no crash, no fabricated progress)FILES_MODIFIED: 3self-report →files_modified=3,has_progress=true(fails onmain)PROGRESS: truewithout a file count →has_progress=true,files_modified=0(fails onmain)FILES_MODIFIED: 0→has_progressstaysfalsetest_exit_detection.batssuite: 72/72 passtest_json_parsing.bats: no failures (RALPH_STATUS anchoring tests unaffected)test_circuit_breaker_recovery.bats: no failures