feat: Wire rate limiting & circuit breaker into watch command (#515)

[tamirdresher]

Summary

Wires the rate limiting SDK (PR #518) into Ralph's watch command polling loop.

Changes

• \�xecuteRound()\ wraps
  unCheck()\ with circuit breaker state machine (closed/open/half-open)
• Predictive circuit breaker opens BEFORE hitting 429 based on quota trend
• Exponential cooldown: 10min → 20min → 40min (capped at 60min)
• \ghRateLimitCheck()\ + \isRateLimitError()\ utilities in \gh-cli.ts\
• 7 integration tests

Fixes (Q Review)

• Race condition guard: Added
  oundInProgress\ flag to prevent overlapping \setInterval\ rounds when a round takes longer than the interval
• Dead code removed: Removed unused \getRetryDelay\ call and import
• Unused import removed: \shouldProceed\ was imported but never used

Architecture Note: Model Fallback

This circuit breaker handles GitHub API rate limits (429 from \gh\ CLI). It is intentionally separate from the SDK's \ModelFallbackExecutor\ which handles LLM model switching (trying cheaper models when premium ones fail). The watch command makes no LLM calls — it uses deterministic triage logic — so model fallback does not apply here. If LLM-based triage is added in the future, \ModelFallbackExecutor\ should be integrated at that point.

Testing

• 29/29 rate limiting tests pass (22 SDK + 7 CLI)
• Full suite: 4818 pass, 0 new failures
• 2 pre-existing failures (same as \dev\ branch)

Closes #515
Depends on #518

[bradygaster]

The circuit breaker state machine and exponential backoff implementation are solid — this is clearly production-informed code. However, the delivery approach needs adjustment:\n\nMain issue: Full rewrite of watch.ts. This PR deletes all 355 lines of the existing watch.ts and replaces them with 534 new lines. The actual new functionality (executeRound wrapper, CircuitBreakerState, circuit breaker lifecycle) is modest — it should be addable as a surgical patch on top of the existing file rather than a full rewrite.\n\nWhy this matters:\n- Creates guaranteed merge conflicts with #520 (which also modifies watch.ts)\n- Removes the existing try/catch error handling from
unCheck, letting all errors from \ghIssueList\ crash the round (behavior change)\n- Makes future watch.ts changes harder to review\n\nRequest: Please rework this as an additive patch — keep the existing watch.ts structure and wrap the circuit breaker around the existing flow. The algorithms are great; just the delivery needs to change.\n\nA few smaller notes:\n- \saveCBState\ is called with \�wait\ but uses \writeFileSync\ — harmless but inconsistent\n- ./core/gh-cli\ export added to CLI package.json exposes an internal module as public API — is that intentional?\n\nHappy to discuss the approach if you'd like to chat about it! 💬

[tamirdresher]

Code Review — Race Condition Analysis

Hi @bradygaster — reviewing PR #522 as a downstream consumer of the Squad framework.

Race Condition Fix ✅

The roundInProgress boolean guard in the setInterval callback is the correct approach for single-threaded Node.js. The check-and-set is synchronous before any await, so there's no atomicity concern. The try/finally guarantees the lock releases even when executeRound throws. Two tests were added that verify both the happy path and the throw-and-release case. This fix is solid.

One Small Issue: await on a sync function

saveCBState is declared as void/synchronous (uses writeFileSync internally), but is called with await in several places:

await saveCBState(squadDir, cbState); // await on void — no-op

This is harmless but misleading. Suggest either:

Option A — make it truly async (preferred if disk I/O matters under load):

async function saveCBState(squadDir: string, cbState: CircuitBreakerState): Promise<void> {
  await fs.promises.writeFile(
    path.join(squadDir, 'ralph-circuit-breaker.json'),
    JSON.stringify(cbState, null, 2)
  );
}

Option B — drop the await at call sites (simpler):

saveCBState(squadDir, cbState); // synchronous, fine for this use case

Test Gap: executeRound State Transitions

The circuit breaker state machine (open → half-open → closed) has no direct tests. The roundInProgress guard tests are great, but the core logic (e.g., "cooldown expires → half-open", "2 consecutive successes → closed") is not covered. Worth adding before merge:

it('transitions to half-open after cooldown expires', async () => { ... });
it('closes circuit after 2 consecutive successes in half-open', async () => { ... });
it('re-opens circuit on 429 in half-open state', async () => { ... });

The executeRound function signature has all the deps injected already — just needs mocks for ghRateLimitCheck and runCheck.

---

The core algorithms here are well-designed. The await writeFileSync is a minor cleanup and the state machine tests are a medium-priority gap. Otherwise this is production-quality rate limiting code.

[tamirdresher]

Closing this in favor of a reworked PR that addresses all feedback:

• ✅ Additive patch — existing
  unCheck, \checkPRs,
  eportBoard\ are completely untouched
• ✅ \saveCBState\ is synchronous — uses \writeFileSync, no misleading \�wait\
• ✅ State machine tests — 16 new tests covering all transitions
• ✅
  oundInProgress\ guard — prevents overlapping setInterval rounds
• ✅ ./core/gh-cli\ export unchanged — rate limit helpers stay internal

New PR incoming from \ amirdresher/515-rate-limit-watch-v2\ branch.