fix(response_analyzer): guard against zero last_output_length (division by zero)

[daegunjhy]

Problem

lib/response_analyzer.sh aborts with a division-by-zero when $RALPH_DIR/.last_output_length contains 0:

local last_length=$(cat "$RALPH_DIR/.last_output_length")
local length_ratio=$((output_length * 100 / last_length))   # last_length=0 -> division by zero

This happens whenever a prior loop wrote a zero/empty output length into .last_output_length (e.g. an interrupted or crashed agent call that produced no output). On the next loop, analyze_response() dies mid-execution and takes the whole loop down — the loop crashes while streaming output:

lib/response_analyzer.sh: line 801: output_length * 100 / last_length: division by 0 (error token is "last_length")

Fix

Compute length_ratio only when last_length is a positive integer. This also guards against an empty or non-numeric file.

     if [[ -f "$RALPH_DIR/.last_output_length" ]]; then
         local last_length=$(cat "$RALPH_DIR/.last_output_length")
-        local length_ratio=$((output_length * 100 / last_length))
-
-        if [[ $length_ratio -lt 50 ]]; then
-            # Output is less than 50% of previous - possible completion
-            ((confidence_score+=10))
+        # Guard against missing/zero previous length (avoids division by zero)
+        if [[ "$last_length" =~ ^[0-9]+$ ]] && (( last_length > 0 )); then
+            local length_ratio=$((output_length * 100 / last_length))
+
+            if [[ $length_ratio -lt 50 ]]; then
+                # Output is less than 50% of previous - possible completion
+                ((confidence_score+=10))
+            fi
         fi
     fi

Behavior

• No change when last_length > 0.
• When the file is 0 / empty / non-numeric, the "declining engagement" heuristic is skipped for that loop (no false completion signal) instead of crashing the loop.

Summary by CodeRabbit

• Bug Fixes
  • Improved robustness of response analysis by safely handling missing, empty, zero, or non-numeric stored output-length values to prevent calculation errors.
• Tests
  • Added regression tests covering problematic stored output-length cases, ensuring the analyzer completes successfully and still generates output.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7bd264fb-2505-453f-a81c-d29a56293a74

📥 Commits

Reviewing files that changed from the base of the PR and between abe93b6 and e8b2147.

📒 Files selected for processing (1)

• tests/unit/test_exit_detection.bats

---

Walkthrough

analyze_response now checks that last_length is a non-zero integer before computing length_ratio, and the test suite adds regressions for 0, empty, and non-numeric .last_output_length values.

Changes

Output Length Guard Fix

Layer / File(s)	Summary
Validate last_length before computing length_ratio lib/response_analyzer.sh	Guards last_length with an integer and > 0 check before the division that produces length_ratio; the confidence increment for declining output is now conditional on those checks passing.
Cover invalid last_output_length values tests/unit/test_exit_detection.bats	Adds regression tests that run analyze_response with .last_output_length set to 0, empty, or non-numeric values and assert successful completion plus creation of .response_analysis.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit tapped the shell with glee,
"No divide-by-zero for me!"
With tests in place, the path stays bright,
And output trends now read just right.
🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: guarding last_output_length to prevent division-by-zero crashes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/test_exit_detection.bats`:
- Around line 1465-1489: The regression test for analyze_response only covers an
empty .last_output_length file, so it does not actually exercise the non-numeric
path. Update the test in test_exit_detection.bats to include a separate real
non-numeric value for .last_output_length in addition to the empty-file case, so
the [[ "$last_length" =~ ^[0-9]+$ ]] guard is validated for both branches. Keep
the coverage centered on analyze_response and the .last_output_length setup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b22e8c0a-47b6-4fba-8c4d-28471e94abb7

📥 Commits

Reviewing files that changed from the base of the PR and between 95425e7 and abe93b6.

📒 Files selected for processing (1)

• tests/unit/test_exit_detection.bats

[daegunjhy]

Code Review — requesting-code-review

Review of the division-by-zero guard fix + regression tests (head abe93b6).

Strengths

• The guard [[ "$last_length" =~ ^[0-9]+$ ]] && (( last_length > 0 )) correctly covers zero / empty / non-numeric .last_output_length, and the unconditional echo "$output_length" > .last_output_length afterward keeps the next loop's input valid.
• The regression tests demonstrate red (crash on the pre-fix base 0c1d7bf) → green (pass with the guard).

Findings

#	Severity	Type	Location	Finding
1	🟡 Minor	Functional correctness	tests/unit/test_exit_detection.bats L1489	The "empty/non-numeric" test writes only an empty file, so the non-numeric branch of [[ "$last_length" =~ ^[0-9]+$ ]] is never exercised with an actual non-numeric value — the test name advertises coverage the body doesn't provide. Suggested: loop over "" "abc". (Raised by coderabbitai; verified valid.)

Rejected (pushback)

• ⚪ merge-commit → linear rebase — Rejected. The PR head being a merge commit (abe93b6 "Merge branch 'red-test'") is a deliberate red→green TDD structure (test 633d70e red on base, fix 95425e7 green). A linear-fix→test rebase preference against that intentional choice is reviewer preference, not a defect — withdrawn.

Assessment

Critical 0 · Important 0 · Minor 1 (valid, quick-win) · Rejected 1. The fix is correct; no merge blocker.

[daegunjhy]

AI Review Summary — receiving-code-review

Reviewer matrix: CodeRabbit (walkthrough + 1 actionable) + Code Review (requesting-code-review, this PR's paired comment). Copilot Code Review is unavailable on this fork PR (no subscription / no historical use) — auto-fallback.

#	Source	Severity	Finding	Status
1	coderabbitai	🟡 Minor	non-numeric branch not exercised — tests/unit/test_exit_detection.bats L1489 writes only an empty file (suggested: loop over "" "abc")	🟡 Deferred (author follow-up)
2	Code Review	⚪ Rejected	PR head merge-commit (abe93b6) → linear rebase	⚪ Rejected — deliberate red→green TDD structure; reviewer preference, not a defect

Verdict

✅ Critical 0 · Important 0 · Minor 1 (deferred, valid) · Rejected 1. The guard covers zero / empty / non-numeric .last_output_length, and the regression tests verify red → green. No merge blocker; the one valid Minor item is a quick-win at the author's discretion.

[frankbria]

Cross-family review — opencode (zai/glm-5.2)

Adversarial Review — PR #333

Guard logic analysis

The condition [[ "$last_length" =~ ^[0-9]+$ ]] && (( last_length > 0 )) is sound for all three crash inputs:

.last_output_length content	regex match	> 0	division skipped?
0	✓	✗	✓ (guarded)
`` (empty)	✗	—	✓
not-a-number	✗	—	✓
5	✓	✓	✗ — normal path runs

The two-layer guard is correctly redundant: ^[0-9]+$ requires ≥1 digit (blocks empty/strings), and > 0 handles the literal-zero case that the regex alone would pass. Layering is necessary, not belt-and-suspenders.

Findings

Suggestion — lib/response_analyzer.sh:804 (criterion 2 test coverage): Acceptance criterion 2 ("positive last length still adds +10") is preserved by the code but not asserted by any test in this diff; the three new tests only cover the no-crash cases. A fourth test with a prior last_output_length=1000 and a short current output asserting confidence_score gained +10 would lock in the unchanged-behavior contract and catch future regressions that silently disable the heuristic. Failure scenario: a later refactor inverts the guard and the +10 path silently dies without any test going red.

Nitpick — lib/response_analyzer.sh:806 (length_ratio scoping): local length_ratio is now declared only inside the guarded branch. The diff shows no later reference, but if any downstream code (echoed to status.json, debug log) reads length_ratio when the guard is false, it would now get an unset/empty value rather than a previously-computed (possibly crashing) one. This is strictly safer than before, just flagging that the variable's lifetime narrowed — not a defect in the shown diff.

Edge cases verified safe

• Leading-zero values (007): bash octal interpretation in (( )) won't crash, only a marginally different ratio; the writer (echo "$output_length") emits decimal so this path is unreachable in practice.
• Whitespace-padded (5) / float (5.0) / negative (-5): all fail ^[0-9]+$, skip the block, degrade gracefully (no +10), no crash.
• cat failure / file present but unreadable: pre-existing behavior, outside this PR's scope.

Security

No new external input surface, no eval, no injection vector introduced. The regex anchor (^...$) prevents partial-match surprises. Clean.

Test quality

The three regression tests are well-constructed: they git init (the function uses git), source the lib fresh per test, exercise each distinct crash input, and use assert_success + .response_analysis existence as a "ran to completion" signal — a meaningful completion proxy, not a tautology.

Verdict

APPROVE

[frankbria]

Final Triage Summary (PR #333)

Cutoff: 2026-07-10T18:59:38Z — no new findings since cutoff.

Applied

• [opencode/GLM, Suggestion] Criterion 2 (+10 output-decline bonus on valid positive last length) had no test → added differential regression test in 179d2ae (score delta between prior-length 1000 vs 1 runs must be exactly 10); mutation-verified (disabling the +=10 fails it).

Skipped (with justification)

• [opencode/GLM, Nitpick] length_ratio scope narrowed into the guarded branch — confirmed non-issue: no other consumer of length_ratio exists in the repo (grep over lib/ and ralph_loop.sh).

CI note

• review / review (GLM workflow) red is the known fork-PR limitation (no secrets available to fork runs) — covered by the manual cross-family review above (verdict: APPROVE). test + coverage green on 179d2ae.

Demo

• Both criteria verified with outcome evidence: main's unfixed lib aborts with "division by 0" and never writes .response_analysis; the PR branch completes analysis for 0/empty/non-numeric last lengths; the +10 heuristic delta is exactly 10 on the normal path.