fix: add re-detection guard to usage_limit handler in onWorkerTerminalState

[lsm]

Summary

• Add !group.rateLimit guard to the usage_limit block in onWorkerTerminalState(), matching the existing rate_limit guard pattern at line 693. This prevents infinite loops when usage limits expire and recoverStuckWorkers re-triggers with stale error text.
• Add unit test verifying re-trigger behavior: when group.rateLimit is already set (even if expired), the usage_limit handler is skipped and falls through to normal routing.

Context

Without this guard, every time onWorkerTerminalState is called with old usage-limit text in the output (e.g., after recoverStuckWorkers fires post-expiry), it re-applies the backoff and returns early — even if the limit has already expired. This caused permanent deadlocks.

Part of the "Fix usage limit handling and fallback model auto-switch gaps" goal.

Test plan

• New unit test: does NOT re-set rate limit when group.rateLimit is already set (usage_limit re-trigger after expiry)
• All 16 terminal-error tests pass
• Full test suite passes (1521 tests)

[lsm]

🤖 Review by glm-5-turbo (GLM)

Model: glm-5-turbo | Client: NeoKai | Provider: GLM

Scope Reviewed

Commit cb7bf9967 — the single commit relevant to Task #11 (plan milestone 1, tasks 1.1 + 1.3). This commit changes two files:

• packages/daemon/src/lib/room/runtime/room-runtime.ts (lines 729-779)
• packages/daemon/tests/unit/room/room-runtime-terminal-errors.test.ts (new test at lines 313-351)

---

Correctness: PASS

The fix is correct and mechanically mirrors the existing rate_limit re-detection guard at lines 687-727. The logic is sound:

1. First detection (group.rateLimit is null): The existing usage_limit behavior runs unchanged — it tries the fallback model first, and if that fails, sets a backoff via setRateLimit, appends a rate_limited event, persists the task restriction, and schedules a tick after reset.

2. Re-trigger after expiry (group.rateLimit is non-null but expired): The guard if (!group.rateLimit) evaluates to false, so the handler body is skipped entirely. The worker falls through to the worktree cleanliness gate at line 782 and then proceeds to normal routing (leader injection). This matches the comment at line 735-736 which explains that the sentinel is intentionally never cleared by the timer — confirmed at scheduleTickAfterRateLimitReset (line 3855-3859) which explicitly states "Do NOT clearRateLimit here." The rate limit is only cleared in send_to_worker (line 1217).

3. No behavioral change for active rate limits: When group.rateLimit.resetsAt is still in the future, isRateLimited() returns true and recoverStuckWorkers (line 2579) skips the group entirely — the usage_limit handler is never reached.

---

Test Quality: PASS

The new test at line 313 follows the exact same pattern as the existing rate_limit re-trigger test at line 232, with appropriate adaptations:

• Creates a context with usage-limit output text ("You've hit your limit")
• Simulates the initial detection via spawnAndSimulateWorkerOutput
• Overrides group.rateLimit to an expired sentinel (mimicking the timer having fired)
• Re-triggers onWorkerTerminalState as recoverStuckWorkers would
• Asserts resetsAt was NOT pushed to a new future timestamp
• Asserts feedbackIteration > 0 confirming routing to the leader occurred

All 16 tests in the file pass. The test adequately covers the critical scenario (expired sentinel + re-trigger).

---

Edge Cases Considered

1. Fallback model success path: When trySwitchToFallbackModel succeeds on first detection, the code falls through to normal routing (line 774-775). On re-trigger with the sentinel set, the guard skips the entire block — which is correct because the fallback switch already happened.

2. Sentinel set by rate_limit handler: If the sentinel was set by the rate_limit block (not usage_limit), and then a subsequent re-trigger encounters usage_limit text, the guard still skips correctly — the sentinel is agnostic to which handler set it.

3. Thread safety / async ordering: The group.rateLimit check and subsequent setRateLimit call are not atomic, but this is acceptable given the single-threaded JS runtime model and the existing pattern at the rate_limit block.

---

P2: Leader usage_limit path is unguarded (not in this PR's scope, but worth tracking)

The leader path at lines 1105-1142 in onLeaderTerminalState() has the same missing guard for usage_limit. While the plan document (Task 1.2) acknowledges this as separate scope and the inline comment at line 1070-1075 explains there is no recoverStuckLeaders mechanism currently, this means:

• If a leader future recovery mechanism is added (plan Gap F), this same infinite-loop bug will surface on the leader path.
• The defensive guard pattern is already established for rate_limit in the leader path (line 1076: errorClass?.class === 'rate_limit' && !group.rateLimit), so adding the same guard for usage_limit would be low-effort and high-value.

This is not a blocker for this PR (the task scope is explicitly worker-only), but it should be tracked as a follow-up before any leader recovery mechanism is built.

---

Summary

The implementation is correct, well-documented with comments, properly tested, and follows the established pattern. No bugs, no security concerns, no architectural issues. The leader-path gap is a known separate task.

Recommendation: APPROVE