fix(observer): auto-scale max_turns by analysis batch size#2062
fix(observer): auto-scale max_turns by analysis batch size#2062zanni098 wants to merge 1 commit into
Conversation
…2035) The hardcoded default of MAX_TURNS=20 is insufficient when MAX_ANALYSIS_LINES=500 (also the default). Claude exhausts its turn budget before it can write all discovered instinct files, producing: Error: Reached max turns (20) Fix: when ECC_OBSERVER_MAX_TURNS is not explicitly set, compute max_turns proportionally to the actual analysis batch size: max_turns = clamp(analysis_count / 10, 20, 100) This gives: - 20–199 lines → 20 turns (existing floor, unchanged) - 500 lines → 50 turns (resolves the reported failure) - 1000 lines → 100 turns (cap) Explicitly setting ECC_OBSERVER_MAX_TURNS still overrides the auto-scaled value, preserving the existing escape hatch.
📝 WalkthroughWalkthroughThe observer loop's ChangesObserver Max Turns Auto-scaling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/continuous-learning-v2/agents/observer-loop.sh (1)
221-229: 💤 Low valueConsider adding a comment explaining the validation block's purpose.
The validation block serves as a safety net for invalid
ECC_OBSERVER_MAX_TURNSinput (e.g., non-numeric or too-low values). While the auto-scaled path always produces valid values ≥20, this block is still necessary for the explicit-override case.A brief comment would help future maintainers understand why this validation remains after the auto-scaling change.
📝 Suggested clarifying comment
fi exit_code=0 + # Validate max_turns in case ECC_OBSERVER_MAX_TURNS was set to invalid value case "$max_turns" in ''|*[!0-9]*) max_turns=20🤖 Prompt for 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. In `@skills/continuous-learning-v2/agents/observer-loop.sh` around lines 221 - 229, Add a brief explanatory comment above the validation block that guards the max_turns value: explain that this checks the user-provided ECC_OBSERVER_MAX_TURNS (stored in max_turns) for non-numeric or too-small values and forces a safe default of 20, and note that although auto-scaling produces values ≥20, the check is needed for explicit overrides; locate the block referencing max_turns and the numeric check (case "$max_turns" in ... and the if [ "$max_turns" -lt 4 ] test) and insert the comment immediately before it.
🤖 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 `@skills/continuous-learning-v2/agents/observer-loop.sh`:
- Around line 208-218: The test assertion expecting the literal string
max_turns="${ECC_OBSERVER_MAX_TURNS:-20}" must be updated to reflect the new
auto-scaling logic in observer-loop.sh: change tests/hooks/hooks.test.js to no
longer look for the removed ":-20" default and instead assert that
observer-loop.sh references the ECC_OBSERVER_MAX_TURNS variable and implements
the fallback autoscale block (checks for ECC_OBSERVER_MAX_TURNS being non-empty,
otherwise computes max_turns from analysis_count and clamps to 20–100). Ensure
the test verifies presence of the ECC_OBSERVER_MAX_TURNS reference and the
autoscale/clamp logic (or matches a regex covering those lines) rather than the
old fixed default literal.
---
Nitpick comments:
In `@skills/continuous-learning-v2/agents/observer-loop.sh`:
- Around line 221-229: Add a brief explanatory comment above the validation
block that guards the max_turns value: explain that this checks the
user-provided ECC_OBSERVER_MAX_TURNS (stored in max_turns) for non-numeric or
too-small values and forces a safe default of 20, and note that although
auto-scaling produces values ≥20, the check is needed for explicit overrides;
locate the block referencing max_turns and the numeric check (case "$max_turns"
in ... and the if [ "$max_turns" -lt 4 ] test) and insert the comment
immediately before it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf9e366d-6d09-4467-a085-4444cfffc05f
📒 Files selected for processing (1)
skills/continuous-learning-v2/agents/observer-loop.sh
| # Auto-scale max_turns proportional to analysis batch size when not explicitly set. | ||
| # The old hardcoded default of 20 is insufficient for the 500-line MAX_ANALYSIS_LINES | ||
| # default: Claude hits --max-turns before it can write all discovered instinct files. | ||
| # Formula: 1 turn per 10 analysis lines, floor 20, cap 100. (#2035) | ||
| if [ -n "${ECC_OBSERVER_MAX_TURNS:-}" ]; then | ||
| max_turns="${ECC_OBSERVER_MAX_TURNS}" | ||
| else | ||
| max_turns=$(( analysis_count / 10 )) | ||
| if [ "$max_turns" -lt 20 ]; then max_turns=20; fi | ||
| if [ "$max_turns" -gt 100 ]; then max_turns=100; fi | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the test still expects the old pattern
echo "=== Checking test expectations ==="
rg -n 'max_turns="\$\{ECC_OBSERVER_MAX_TURNS:-20\}"' tests/hooks/hooks.test.js
echo ""
echo "=== Checking if test was recently modified ==="
git log -1 --oneline -- tests/hooks/hooks.test.jsRepository: affaan-m/ECC
Length of output: 354
Fix failing hook test assertion (old :-20 string removed)
tests/hooks/hooks.test.js still asserts that observer-loop.sh contains the exact string max_turns="${ECC_OBSERVER_MAX_TURNS:-20}", but the new logic auto-scales max_turns (clamped to 20–100) instead of using that fixed :-20 default—so CI will fail when the literal is no longer present.
🤖 Prompt for 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.
In `@skills/continuous-learning-v2/agents/observer-loop.sh` around lines 208 -
218, The test assertion expecting the literal string
max_turns="${ECC_OBSERVER_MAX_TURNS:-20}" must be updated to reflect the new
auto-scaling logic in observer-loop.sh: change tests/hooks/hooks.test.js to no
longer look for the removed ":-20" default and instead assert that
observer-loop.sh references the ECC_OBSERVER_MAX_TURNS variable and implements
the fallback autoscale block (checks for ECC_OBSERVER_MAX_TURNS being non-empty,
otherwise computes max_turns from analysis_count and clamps to 20–100). Ensure
the test verifies presence of the ECC_OBSERVER_MAX_TURNS reference and the
autoscale/clamp logic (or matches a regex covering those lines) rather than the
old fixed default literal.
Fixes #2035.
Problem
observer-loop.shhardcodesmax_turns=20as the default for the Claude analysis subprocess, whileMAX_ANALYSIS_LINESdefaults to500. The two defaults are mismatched: Claude exhausts its turn budget before it can finish reading all observations and writing all qualifying instinct files.Fix
When
ECC_OBSERVER_MAX_TURNSis not explicitly set, auto-scalemax_turnsproportionally to the actual analysis batch size (analysis_count, which is already computed just before this code):Results:
analysis_countmax_turnsSetting
ECC_OBSERVER_MAX_TURNSexplicitly still overrides the auto-scaled value, preserving the existing escape hatch.Why not just raise the hardcoded default to 50?
Auto-scaling is strictly better: it handles any value of
ECC_OBSERVER_MAX_ANALYSIS_LINEScorrectly, whereas a new hardcoded default of 50 would still fail if a user cranksMAX_ANALYSIS_LINESbeyond 500.Files changed
skills/continuous-learning-v2/agents/observer-loop.sh— replace 3-linemax_turnsassignment with an 8-line auto-scaling blockTesting
Manual verification of the logic (pure shell arithmetic, no external deps):
Summary by cubic
Auto-scales the observer’s
max_turnsbased onanalysis_countso Claude doesn’t hit the turn limit before processing the default 500-line batch and writing all instinct files.max_turns = clamp(analysis_count / 10, 20, 100)whenECC_OBSERVER_MAX_TURNSis not set.ECC_OBSERVER_MAX_TURNS.Written for commit 8dff671. Summary will update on new commits. Review in cubic
Summary by CodeRabbit