🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: REQUEST_CHANGES

Reviewed from scratch: full diff, all 17 changed files, the integration points (query-runner.ts, model-switch-handler.ts, processing-state-manager.ts), and the surrounding space-runtime tick/rehydrate paths. Ran the new/changed unit suites (53 + 70, all green) and bun run check (lint/typecheck/knip/parity/session-guards/test-quality — all clean). The coder's verification claims hold.

What's solid. The pure recovery module (fallback-recovery.ts) is well-factored and correctly handles the Chinese relay shape (2026-07-22 17:55:10 via LOCAL_DATETIME_RE, the [1308] code correctly ignored), the backoff ladder, and the freeWait semantics. The concurrency core is sound: switchAndRetryForFallback correctly await this.queryPromise so the failed query's finally (null queryObject, env restore, setIdle) completes before handleModelSwitch runs — I traced no double-query / dedup / late-finally race, and cross-provider switching works (config-only branch swaps provider + clears sdkSessionId). Migration 163 follows the established M98/M162 table-rebuild pattern (FK-off, column copy, idempotent guard) and is safe. The onMarkApiSuccess → reset() → notifyResume resume path is correctly wired.

The findings below are real and actionable but none are crash/incorrect-output bugs — they concern reliability of the feature's headline guarantee and test coverage of its central invariants.

P2-1 — Auto-resume does not survive a daemon restart (the motivating use case)

The cooldown is an in-memory unref'd setTimeout (rate-limit-watchdog.ts:295). The persisted restrictions.resetAt blob (migration 163 + repo round-trip) is written on pause but never read on startup. The space-runtime tick loop only drives open/in_progress/blocked tasks (space-runtime.ts:4066), so a usage_limited/rate_limited task is never re-driven, and rehydrating a session constructs a fresh watchdog with no timer/retryCount.

Consequence: for a 5-hour or weekly cap — the exact scenario this PR exists to fix — a daemon restart during the wait leaves the task paused indefinitely with no auto-resume and no error. This is the same "persisted data with no runtime consumer" anti-pattern this PR removes for fallbackModels; the new resetAt repeats it. The manual Resume button (TaskStatusActions.tsx: rate_limited->in_progress) is an escape hatch, so it is recoverable, not a hard-stuck — but the "auto-resumes after reset" criterion (Part C / VERIFICATION) is unmet across restarts, and a paused task with a future resetAt that nobody arms is misleading in the UI.

Fix options: on daemon/workflow start, scan paused tasks — those with resetAt in the past → restore in_progress + clear restrictions; those still future → re-arm a runtime-level scheduled restore (or re-arm the watchdog cooldown on rehydrate). At minimum, explicitly document that auto-resume requires daemon uptime. The persisted resetAt should either be consumed or not persisted as a resume promise.

P2-2 — fireImmediateFallback has no error boundary; a rejection sticks fallbackPending = true

rate-limit-watchdog.ts:321-339 is void-fired from scheduleRetry:239 with no try/catch. switchAndRetryForFallback has a catch-all, but its own catch calls this.stateManager.setIdle() which can throw (DB write); the recursive await this.scheduleRetry(...) at :337 can also reject. If anything escapes, the promise rejects unhandled AND this.fallbackPending is never reset (:326 is skipped), so getState() reports 'fallback-pending' forever and retryNow() (:365) hard-returns false — a stuck-visible state with no recovery short of reset(). Wrap the body in try/catch: on error, log, clear fallbackPending, mark the entry tried, and fall through to a cooldown.

P2-3 — Untested load-bearing invariants

Three behaviors central to this feature have no test, so they will silently regress:

• Parsed-reset waits bypass maxAutoRetries. This is the design guarantee ("reset-known waits don't count toward the budget"). The only exhaustion test (rate-limit-watchdog.test.ts:159) uses '429' (no timestamp) and asserts false; nothing asserts that with retryCount === maxAutoRetries + a parseable ISO reset, scheduleRetry returns true and retryCount stays put.
• Late resume does not resurrect a cancelled/done task. Explicitly in the review checklist. The source guard (restoreTaskFromRateLimit:610) is correct, but the only resume tests cover usage_limited→in_progress (:99) and the in_progress no-op (:141); nothing publishes session.rate_limit_resume against a cancelled/done/archived task. The terminal-override test (:129) only covers done on the pause side.
• resetAt buffer arithmetic on the notifyPause payload is asserted nowhere (watchdog emits decision.retryAtMs = reset + 30s, :285); a regression dropping the buffer passes every test.

P3 — optional polish (not blocking on their own)

• rate-limit-watchdog.ts:283 fires notifyPause before setRateLimitCooldown (:289); reorder so the processing-state flip precedes the bus event and avoids a brief idle window where onIdleCallback runs.
• rate-limit-watchdog.ts:237 comment says scheduleRetry "must return true synchronously" — it is awaited at query-runner.ts:1314; rephrase to "must resolve to true".
• fallback-recovery.ts:301-312 classifyLimitKind keywords are broad ('exceeded', 'limit reached') and any parsed timestamp → usage_limit, so a transient "rate limit exceeded, retry in 60s" is labelled a daily/weekly cap. Label-only impact (both resume identically), but worth narrowing.
• A manual status transition out of rate_limited/usage_limited via setTaskStatus (not the resume path) leaves a stale restrictions blob; clear it on exit. No frontend consumer yet, latent.
• VALID_SPACE_TASK_TRANSITIONS omits rate_limited/usage_limited → archived (every other non-terminal status can archive). Likely intentional; confirm.

Happy to re-review once P2-1 through P2-3 are addressed.

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE

Round 2 (fresh, commit 755f340). All three P2 findings and the P3 polish are correctly addressed; I re-verified each on its merits rather than trusting the summary.

P2-1 (cross-restart auto-resume) — fixed. SpaceRuntime.recoverRateLimitedTasks() runs in the tick loop off the persisted restrictions.resetAt: past-reset → restore in_progress + clear restrictions (worker re-driven by the normal in_progress rehydrate/recoverStalledRuns path, which has separate coverage); future-reset → left paused. listRateLimitedBySpace + the repo's auto-clear of stale restrictions on any exit transition back it up. The only residual — a backoff-path rate_limited task carries a synthetic resetAt ≈ now+1h rather than the exact ladder delay — is safe: usage caps (the long-window case) always carry an accurate parsed reset, and a transient rate limit retrying slightly early is harmless. Not worth blocking.

P2-2 (error boundary) — fixed. fireImmediateFallback now wraps switchAndRetry in try/catch with a finally that always clears fallbackPending, and guards the recursive scheduleRetry with a best-effort cooldown fallback. No path leaves fallbackPending stuck or getState()/retryNow() frozen.

P2-3 (invariant tests) — fixed, and the assertions are meaningful: parsed-reset wait bypasses maxAutoRetries (returns true with retryCount unchanged at the budget); the 30s RESET_BUFFER_MS is pinned on the notifyPause payload; late resume does not resurrect cancelled/done/archived and confirms the restrictions auto-clear; pause-side terminal guard broadened to done/blocked/cancelled/archived.

P3 — all applied: setRateLimitCooldown before notifyPause; "must resolve to true" comment; classifyLimitKind narrowed (dropped the over-broad exceeded/limit reached/达到; the Chinese relay still classifies correctly via 上限/小时 and the parsed-reset path); rate_limited/usage_limited → archived transitions + labels.

Verification: bun run check clean (lint/typecheck/knip/parity/session-guards/test-quality); changed suites green (watchdog+fallback 55, listener+tick-loop+repo 125); all 4 prior threads resolved; PR open and mergeable. No new issues or regressions introduced by the fix commit. Ship it.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc71f95761

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e05f3c8620

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65e56e1b50

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2e6c610ce

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 286dcc63aa

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de5c525f6d

🤖 Review by glm-5.1 (zai)

Model: glm-5.1 | Client: NeoKai | Provider: zai

Recommendation: REQUEST_CHANGES — not for logic, which I re-verified as sound, but for a merge-conflict with dev that must be resolved before merge. All 15 bot findings are already fixed.

A. Codex bot findings (reviews 4824157940 + 4824868845) — 15/15 ALREADY RESOLVED on de5c525f6

I independently verified every one of the two Codex review batches against the current head (7 parallel verification passes, each with code + commit evidence). All 15 are resolved, most by b2e6c610c ("3rd-round review") and 65e56e1b5 ("2nd-round review"), each backed by unit tests. Codex review 4824868845 was run against commit_id e05f3c862 (the grandparent of the fixes), which is why its findings landed after the fixes — they are stale, not new regressions.

Logic verdict: APPROVE. No outstanding logic/behavior issues. (Minor, non-blocking note: the per-task limitedSessionsByTask tracker is in-memory; on a restart between two sub-session resumes it's lost — but the persisted restrictions.resetAt is now the max of all paused sessions, so recoverRateLimitedTasks still cannot restore early. No correctness hole.)

B. ⚠️ Real blocker: merge conflict with dev (needs rebase)

GitHub reports mergeable: CONFLICTING. The branch is 12 commits behind dev with conflicts in 3 files:

• packages/daemon/src/lib/space/runtime/space-runtime.ts ← overlaps our recoverRateLimitedTasks / spawn-guard / tick code
• packages/daemon/src/lib/space/runtime/space-runtime-service.ts
• packages/daemon/src/storage/schema/migrations.ts ← our M163

The conflict is driven mainly by:

• #2289 refactor(space): make task lifecycle the only external-event delivery gate — a space-runtime refactor most likely to require our recoverRateLimitedTasks/spawn-guard logic to be re-fit to the new delivery-gate model.
• #2282 / #2284 / #2287 normalized message-replacements refactor — touches the SDK-message path where our onMarkApiSuccess/isSDKResultSuccess gating lives.

After resolving, please re-run the full daemon shards (not just touched files — this touches the tick loop + migrations, the same surface that bit us in an earlier round):

./scripts/test-daemon.sh 1-core
./scripts/test-daemon.sh 5-space-runtime-a   # tick loop
./scripts/test-daemon.sh 4-space-migrations-b # M163

then push and let CI re-run. No --delete-branch (per convention).

Summary: The rate/usage-limit + fallback-chain feature is solid — every external bot finding is already addressed with tests. The only thing standing between this PR and merge is the dev rebase. Once conflicts in the 3 files above are resolved and the daemon shards are green, this is good to merge.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 568183b925

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: APPROVE (round 8, head b6e1ea88a)

Independently verified all 7 codex fixes (3 P1 + 4 P2) plus the two f2135c222 P2s from scratch against the current worktree — not the diff alone. All correct; tests green.

P1 concurrency hardening (rate-limit-watchdog.ts):

• scheduleRetry captures entryGeneration synchronously before the first await (:249), re-checks it before both side effects (fallback switch :314, cooldown schedule :359), and passes the entry generation — not this.generation — into fireImmediateFallback (:332). Closes the mid-resolution cancel race. ✓
• fireCooldownRetry captures generation at entry (:436) and aborts (no notifyResume, no re-arm) if superseded during the awaited retry callback (:450). Post-timer async window closed; no double-resume / dangling timer. ✓
• cancel(notifyResume=true) flag; banner Cancel passes false so it no longer falsely restores the task to in_progress (which the completion listener could misread as node completion). ✓

Exhaustion lifecycle (f2135c222 + b6e1ea88a): terminal startup-retry exhaustion no longer calls notifyResume (it was a false-in_progress orphan) and instead sets startupExhausted, so retryNow admits a manual Resume even without a pending timer. I traced every recovery path — manual Resume, the per-tick recoverRateLimitedTasks sweep (driven by persisted restrictions.resetAt, skips live sessions via isSessionInMemory), and daemon restart. Not stuck. I also checked the banner-Cancel recovery edge case: although cancel(false) severs the in-process retryNow/resumeRateLimitedSubSession paths (state→idle, startupExhausted cleared), the primary affordance — manual Resume via recoverWorkflowBackedTask — restores the task to in_progress in its transaction (:5566, gated only by isValidSpaceTaskTransition(rate_limited→in_progress)) independent of cooldown state, so the task is fully recoverable in-process; resumeRateLimitedSubSession is only an optimization to break a live cooldown immediately. ✓

Mechanical correctness:

• Fractional-zoned regex lookahead (f2135c222) now rejects .<digit> after seconds, so a stale fractional-zoned timestamp the ISO pass dropped is no longer re-accepted as a future local datetime. Bare local shapes (incl. the Chinese relay message) unaffected. ✓ (42/0)
• Migration renumbered M164→M167; chain 160–167 contiguous, marker-gated, idempotent, no-FK comma fix preserved. ✓ (2335/0). Minor: no dedicated M167 unit test — coverage gap, not a defect.
• SpaceOverview Action/Review count now uses the shared isActionRequired predicate. ✓ (22/0)

Merge state: merge-base == origin/dev (49d371961 — fully caught up incl. dev's #2313 artifact-shapes), 0 conflict markers, MERGEABLE / mergeStateStatus CLEAN, 0 unresolved threads / 62, required CI checks pass. Ready for human merge.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6e1ea88a2

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: REQUEST_CHANGES — supersedes my round-8 APPROVE on b6e1ea88a.

Codex posted 4 new findings (all P1) after my round-8 approval. I verified each first-hand against the worktree — all four are real, and they share one root cause.

Root cause: the episode-generation guard pattern (sound, and correctly applied at the watchdog's top-level boundaries this round) is incomplete. It re-checks generation only at scheduleRetry (:314/:359), fireCooldownRetry (:450), and fireImmediateFallback (:537/:559). But the side effects that actually mutate the session live inside the awaited callbacks/sub-methods, where the captured generation is never re-checked:

1. P1 — fireCooldownRetry :443 callback replays the stale message. The callback is executeRateLimitAutoRetry, which calls startQueryAndEnqueue() (agent-session.ts:961) with no generation check. A cancel/interrupt during the callback's setIdle await (:958) bumps the generation, but startQueryAndEnqueue proceeds — the stale message is enqueued and the query started before the :450 check runs (which then only suppresses resume/re-arm). The interrupt is defeated; the stale turn runs. [discussion_r3700228396]

2. P1 — fireImmediateFallback :545 switch commits before the :559 guard. switchAndRetryForFallback awaits the failed query's teardown (:879), then performs handleModelSwitch (:899) and executeRateLimitAutoRetry (:912) inside the await — none check generation. A cancel during the teardown await bumps generation, but the provider switch + stale replay already commit before :559 (which only blocks chain advancement). [discussion_r3700228397]

3. P1 — fireImmediateFallback :572 re-entry treats a cancelled turn as fresh. After the :559 guard passes, await resolveModelId (:572) opens a window. cancel() bumps generation and leaves lastUserMessage populated (it clears episodeMessageUuid but not lastUserMessage). After the await, triedKeys.add (:573) mutates state and scheduleRetry (:579) captures the newly-incremented generation as its baseline — so the stale message is treated as a fresh valid episode and another fallback is selected or a cooldown armed. [discussion_r3700228399]

4. P1 — scheduleCooldown :387 publishes a stale pause + arms a stale timer. After scheduleRetry's :359 guard, scheduleCooldown awaits setRateLimitCooldown (:387). A cancel during that await bumps generation, but cancel() sees a null timer and an unpublished pause (notifyPause at :398 hasn't run) — nothing to undo. After the await, notifyPause (:398) publishes a stale pause (overriding the cancel's resume) and a stale cooldown timer is armed (:404) that later fires into fireCooldownRetry and replays the message. [discussion_r3700228402]

Severity rationale: same class as the P1-A fix accepted this round (scheduleRetry entry-generation capture). The PR's explicit goal is interrupt-safe recovery, and these four windows each let an explicit cancel/interrupt be defeated and the stale turn replayed — immediately (1, 2) or deferred via an armed timer / re-entered recovery (3, 4). Rate-limit recovery is precisely when users interrupt, so the windows are practically reachable.

Fix direction (one coherent change at 4 sites): propagate the captured episodeGeneration into each callback/sub-method and re-check it immediately before every side effect — before startQueryAndEnqueue in executeRateLimitAutoRetry; after the teardown await and before handleModelSwitch/executeRateLimitAutoRetry in switchAndRetryForFallback; after resolveModelId and before triedKeys.add/scheduleRetry re-entry in fireImmediateFallback; and after the setRateLimitCooldown await and before notifyPause/timer-arm in scheduleCooldown. For startQueryAndEnqueue's genuine-new-user-input path the check must stay opt-in via the recovery call sites, not the shared method — consistent with the existing clearPendingCooldown vs cancel split at agent-session.ts:807-814.

The non-concurrency fixes (exhaustion lifecycle, fractional-zoned regex, M167 migration, overview filter) remain correct; only the interrupt-safety of the recovery path needs completing.

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: APPROVE (round 9, head c886ffab2)

Verified the 4 generation-threading P1 fixes first-hand against the worktree. All correct — each guard sits exactly after its await and immediately before its side effect, with no await in between (the gap that defeated the prior boundary-only guards). Threading confirmed: RateLimitRetryCallback/switchAndRetry/scheduleCooldown now carry episodeGeneration, and the captured value (not this.generation) is propagated at every call site — fireCooldownRetry:460, scheduleRetry:368, fireImmediateFallback:562 — so the guards actually fire.

1. executeRateLimitAutoRetry (Finding 1) — setIdle await (:979) → isSuperseded(episodeGeneration) check (:986-992) → startQueryAndEnqueue (:995). Opt-in via episodeGeneration !== undefined, so the shared enqueue path (genuine new input) is untouched. ✓
2. switchAndRetryForFallback (Finding 2) — guard after the teardown await queryPromise (:893, before handleModelSwitch) AND after handleModelSwitch (:916, before re-enqueue). An abort here returns false and the post-switch guard at :576 catches it. ✓
3. cancel() clears lastUserMessage (Finding 3) — verified SAFE: I traced every recovery path. retryNow after cancel is unreachable (cancel nulls the timer + clears startupExhausted, so its own gate short-circuits); space-task manual Resume goes through recoverWorkflowBackedTask's DB transaction (restores task + node executions, re-drives via the tick using the task prompt — independent of the watchdog's lastUserMessage); the restart sweep is restrictions.resetAt-driven (in-memory lastUserMessage is lost on restart anyway); non-space sessions keep the message in SDK history (only auto-replay is forfeited, the correct semantic for an explicit cancel); exhaustion doesn't call cancel, so post-exhaustion Retry Now still works. ✓
4. scheduleCooldown (Finding 4) — setRateLimitCooldown await (:393) → episodeGeneration !== this.generation re-check (:403-408) → notifyPause (:415) + timer arm (:421). A cancel during that write (null timer, unpublished pause) no longer commits either. ✓

One residual examined and cleared (not a finding): fireImmediateFallback:590 can add a stale triedKeys entry if a cancel lands during the resolveModelId await (:589) after the :576 guard passed. This is benign — that path is only reached when the switch genuinely failed (a Fix-2 abort returns at :576 before :583), so the entry is a just-failed model key (skipping it later is harmless), and triedKeys is cleared on the next new-UUID episode (:273) and on reset (:752). The replay itself is already blocked by Fix 3 (:594 if(this.lastUserMessage) is false). No action needed.

Prior fixes intact (scheduleRetry :251/:316/:361, fireCooldownRetry :453/:467, fireImmediateFallback :554/:576, cancel flag, startupExhausted lifecycle, exhaustion no-resume). Tests: watchdog 34/34 (4 new), agent-session 123/123 per the handoff.

Merge state: merge-base == origin/dev (49d371961), 0 conflict markers, MERGEABLE/mergeStateStatus CLEAN, 0 unresolved threads / 66, CI green (0 failures; CLI skips on PR branches as expected). Ready for human merge.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c886ffab21

🤖 Review by GLM-5.1 (GLM)

Model: GLM-5.1 | Client: NeoKai | Provider: GLM (Zhipu)

Recommendation: APPROVE (round 10, head 7372173e6) — the generation-guard concurrency story now reaches the irreducible commit point.

Verified codex's 2 P1 fixes first-hand:

Fix 1 — guard at the lifecycle commit point (QueryLifecycleManager.startQueryAndEnqueue :655-706). await ensureQueryStarted() (:662) → isRateLimitEpisodeSuperseded(episodeGeneration) re-check (:668-677, returns/aborts) → all three commit paths (setQueued blocked :679, setQueued :689, enqueueWithId :693) are AFTER the guard with no await between. Confirmed: enqueueWithId is the irreducible bottom (the message-queue commit). Defense-in-depth with the round-9 :988 guard; episodeGeneration threaded correctly — executeRateLimitAutoRetry:999 → startQueryAndEnqueue:818 → lifecycle :670. ctx wiring correct: new QueryLifecycleManager(this) (:402) → AgentSession.isRateLimitEpisodeSuperseded (:1603) → rateLimitWatchdog.isSuperseded. Opt-in confirmed: the genuine-input caller event-subscription-setup.ts:123 omits the arg → undefined → unchecked. ✓

• The :755/:759 setQueued+enqueueWithId is in handleQueuedMessageFailure (a delivery-timeout retry, gated on MessageQueueTimeoutError :720) — a different mechanism, not governed by the rate-limit episode generation. Out of scope for this PR's interrupt-safety goal; not a finding.

Fix 2 — startup-retry rearm guard (fireCooldownRetry :511-531). setRateLimitCooldown await (:511) → entryGeneration !== this.generation re-check (:523, returns) → timer arm (:531), no await between. Mirrors the scheduleCooldown guard from round 9; closes the sibling window I didn't verify last round. ✓

Two P2s deferred — acceptable per independent assessment, but ⚠️ the tracking citation is wrong. I concur both are deferrable: (a) banner-Cancel in-process recovery is recoverable via restart + recoverWorkflowBackedTask (I traced this in round 8 — the task is restored to in_progress by the DB tx independent of cooldown state); (b) parallel-session restriction recompute is a refinement of a feature this PR introduces (strictly safer, no regression). However, #812 is an unrelated, already-merged "short IDs in task cards" ticket, and an open-issue search finds nothing covering either P2 — so they are currently untracked. Please file a real tracking issue (or correct the number) so these aren't lost. I'm fine leaving them out of this PR.

Prior fixes intact; tests green per handoff (watchdog 34/34, agent-session suite). merge-base == origin/dev, 0 conflict markers, 0 unresolved threads.

Note: CI on 7372173e6 was still running at review time (3 jobs pending, 0 failures). I'll confirm green before the merge handoff.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7372173e66

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE (own-PR, so posted as COMMENT — this is an explicit approval verdict).

Round-11 verification (first-hand, current head 9be5fd1bd)

I re-verified both fixes from this round from scratch, plus the one open question I'd flagged.

P1 — parent-task deferral gate (task-agent-manager.ts:3627-3639) — confirmed effective.
The new gate defers an injected external-event/peer-handoff message when the parent task is rate_limited/usage_limited. I raised whether findParentTaskIdForSubSession (:3729) — which iterates the in-memory this.subSessions map and returns null on miss — actually works in the cross-restart window this fix targets (rehydrate flips persisted rate_limit_cooldown → idle, so inRateLimitCooldown alone is false until the tick sweep re-arms the cooldown).

It holds. injectMessageIntoSession(session: AgentSession, …) only ever runs against an already-in-memory session, and a session reaches memory solely via createSubSession (:1393) or rehydrateSubSession (:3355) — both populate this.subSessions (taskId → sessionId) before the session can process a message. So the lookup is guaranteed to resolve for any session passed in, and parentTask = taskRepo.getTask(parentTaskId) is a DB-backed read → authoritative status. Across the post-restart sub-windows the gate behaves correctly: pre-sweep (parent still rate_limited, session idle) → deferred via parentLimited; post-sweep-future-resetAt → deferred via inRateLimitCooldown; post-sweep-past-resetAt (task restored) → delivered, which is correct. The in-memory implementation is coherent with the in-memory precondition, not a gap.

P2 — fireImmediateFallback recheck (rate-limit-watchdog.ts:605-608) — confirmed.
Re-checks episodeGeneration !== this.generation after the resolveModelId await (:599) and before triedKeys.add (:609) / the scheduleRetry re-entry (:615), so a superseding turn during the await can no longer poison the new episode's triedKeys set after its clear (:273).

Convergence assessment

Agree the generation-guard concurrency theme is now closed. All side-effect commit points carry the captured-episodeGeneration re-check on this head:

• watchdog: scheduleCooldown (:403), fireCooldownRetry (:564), startup-retry (:523→:531), fireImmediateFallback re-entry (:586) + post-resolve (:605)
• agent-session: switchAndRetryForFallback post-queryPromise (:894) / post-handleModelSwitch (:917); executeRateLimitAutoRetry (:989); delegation (:1603)
• QueryLifecycleManager.startQueryAndEnqueue commit-point guard before enqueueWithId (:670), ctx-wired + opt-in (:94)

CI / merge state on 9be5fd1bd

All CI checks SUCCESS (lint/typecheck, full daemon online+unit matrix, web tests, coverage gate, "All Tests Pass"), 4 matrix jobs SKIPPED (non-triggered). PR OPEN / MERGEABLE / CLEAN / base dev / not draft. All 73 review conversations resolved (0 unresolved, 0 open-and-current).

Residual nit (non-blocking, P3)

The deferred-P2s are acceptable to defer — no correctness/safety impact, and they're edge-case hardening. One repeat note (carried from prior rounds, still non-blocking): the tracking ticket cited for them ("#812") maps to an unrelated merged "display short IDs in task cards" PR in this repo, so the citation is ambiguous to outside readers. If it refers to an internal NeoKai space task, a one-word qualifier ("space task #812") would remove the ambiguity. Not gating.

Verdict

All P0–P3 findings from this and prior rounds are either fixed and re-verified or explicitly deferred with acceptable rationale. CI is green, merge state is clean, every review conversation is resolved. Approving.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9be5fd1bde

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE (own-PR, so posted as COMMENT — this is an explicit approval verdict).

Round-12 verification (first-hand, head c1c751ebd) — codex round-6 fixes

P1 — supersede recovery on genuine new input (agent-session.ts:807-829) — confirmed correct.
The branch on episodeGeneration is the right root-level fix for the design flaw the prior generation-guard findings circled:

• Genuine input (undefined) → watchdog.cancel() — bumps generation so an in-flight fallback switch / cooldown-retry callback (which captured the old generation) sees isSuperseded() and aborts before switching models or replaying the stale message. cancel() also clears the timer/episode message.
• Internal recovery (episodeGeneration provided) → clearPendingCooldown() — clears only the timer, preserving generation + episode so the in-flight fallback and per-episode tried-set survive.

I traced every caller to confirm the wiring: genuine-input paths omit episodeGeneration (message-persistence.ts:264 — the path the finding named, event-subscription-setup.ts:123, session-handlers.ts:1300, space-agent-tools.ts:1213) → cancel; the sole recovery caller (agent-session.ts:1008 inside executeRateLimitAutoRetry, behind the supersede guard at :996-1002) passes it → clearPendingCooldown. No recovery caller can self-cancel; no genuine caller can fail to supersede.

Two correctness details checked: (1) notifyResume() guards on if (!this.paused) return (:805), so an ordinary user message with no active cooldown produces no spurious resume event — the cancel path is a no-op for resume there. (2) A genuinely new UUID triggers the per-episode reset in scheduleRetry (:276-283, clears triedKeys/chain/retryCount/startupRetries), so the new turn gets the full fallback chain rather than the prior turn's exhausted one. This also closes the related "resume the task when new input replaces a cooldown" P2 — cancel()'s notifyResume ends the old pause.

P2 — TransientSpawnError for limited-task spawn — confirmed correct.
Delegated a focused trace. The spawn-loop catch (space-runtime.ts:8086-8130) orders guards correctly: cancelExecutionForPermanentSpawnError early-returns false for non-permanent (:8200), then isTransientSpawnError → continue (:8097-8101) lands before the generic crash-retry path (:8117), so a transient cap neither consumes a MAX_TASK_AGENT_CRASH_RETRIES budget nor cancels/unregisters the execution. No infinite-loop or orphan risk: processRunTick early-returns for rate_limited/usage_limited tasks (:7619-7621) so the tick never re-spawns while paused, and recoverRateLimitedTasks restores (resetAt passed) → next tick re-spawns the pending execution. TransientSpawnError extends Error (not PermanentSpawnError), instanceof checks are isolated, no property-style checks exist. The path is only reachable via out-of-band activation (tick early-returns first), matching the validator's doc comment.

P2 — full backoff ladder reachable — confirmed correct.
BACKOFF_LADDER_MS = [10m, 30m, 1h, 2h, 4h] (fallback-recovery.ts:27-33, 5 entries; BACKOFF_CAP_MS=8h so the 4h step isn't clipped). maxAutoRetries: BACKOFF_LADDER_MS.length (=5). Budget boundary (!freeWait && retryCount >= maxAutoRetries) lets retryCount 0→4 select ladder[0..4] (all five steps) before going terminal at 5; !decision.freeWait gates both the budget check and the increment, so parsed-reset waits stay free. Exactly the intent.

Deferred findings (the 3 codex posted at 21:23)

Not actually outstanding on this head:

• P1 "Preserve cooldown when lazily rehydrating sessions" (task-agent-manager.ts:3627) — this is the round-11 parent-task gate (parentLimited at :3634, gate at :3636), present on this head. Fixed.
• P2 "Recheck the episode after canonicalizing a failed fallback" (rate-limit-watchdog.ts:~610) — the post-resolveModelId recheck before triedKeys.add, present on this head. Fixed.
• P2 "Revalidate dependencies before resuming a limited task" (task-agent-manager.ts:753) — genuinely deferred edge case (add unmet dependency mid-cooldown, resume ignores it). P2-level, not safety-critical; acceptable to track in #811/#812.

CI / merge state on c1c751ebd

All required checks SUCCESS ("All Tests Pass", 0 failures, 0 incomplete). PR OPEN / MERGEABLE / CLEAN / base dev / not draft. All 73 review conversations resolved (0 open).

Residual nits (non-blocking, P3)

1. No unit test for the TransientSpawnError path. Existing spawn-failure tests cover only PermanentSpawnError. Given the subtle error-type ordering (transient continue before crash-retry, out-of-band-only reachability) and the handshake with recoverRateLimitedTasks, a regression test asserting (a) a limited task's execution stays pending after an out-of-band activation, and (b) re-spawns after recovery, would be worth adding. Coverage recommendation, not a defect.
2. #811/#812 citation (carried from prior rounds): the "remaining task-lifecycle follow-ups" tracking ticket citation remains ambiguous to outside readers — please confirm it refers to internal space tasks (not GitHub issues) so the deferral is traceable. Not gating.

Verdict

The root design flaw (genuine-input supersede vs. internal-recovery re-enqueue conflated at startQueryAndEnqueue) that drove much of the round 2–6 generation-guard back-and-forth is corrected at its source, and all three round-6 fixes verify first-hand. All P0–P2 findings are fixed-and-verified or acceptably deferred; only two P3 coverage/process nits remain, both explicitly non-blocking. CI green, merge state clean, every review conversation resolved. Approving — convergence is the right call here; the human can weigh the #811/#812 task-lifecycle follow-ups against shipping now.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1c751ebd0

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE (own-PR, so posted as COMMENT — this is an explicit approval verdict).

Round-13 verification (first-hand, head 2b612a7d6) — codex round-7 fixes

P1 — route resettable quota 429s through recovery (fallback-recovery.ts:351-378 + query-runner.ts:1326-1334) — confirmed correct.
This was a genuine primary-case feature gap: the old inline isBillingError matched quota exceeded / insufficient_quota / no quota unconditionally, so a 429 quota exceeded … resets at <ts> was terminal-billing and onRateLimitExhausted was never called — leaving the reset parser + usage-limit classification unreachable for English quota-429s. The extracted isNonRetryableBillingError carves out the resettable case: 402 is always billing; quota phrases are billing unless extractResetTimestamp finds a reset window, in which case it routes to recovery.

The safety of this hinges on no false-positive rescuing a genuine billing error — and it holds: extractResetTimestamp returns non-null only when some candidate passes isValidReset, which is ms > now && ms < now + MAX_RESET_HORIZON_MS (strictly future, within horizon). So a billing error with no reset, or with only a past request timestamp, yields resettable=false → billing. 4 tests pin the matrix: 402-always-billing (even with a timestamp), quota-no-timestamp→billing, quota+timestamp→recovery, plain-429→not-billing. The common case (429 without quota phrases) is unchanged (isNonRetryableBillingError → false → recovery).

P2 — manual resume uses watchdog state (agent-session.ts:1045-1055 + task-agent-manager.ts:2251-2266) — confirmed correct, a real improvement.
resumeRateLimitedSubSession no longer pre-checks the volatile rate_limit_cooldown processing state (which query-runner's unconditional setIdle() overwrites to idle on the failed query's finally, so manual resume usually reached the check idle and skipped the retry). retryNowAfterRateLimit now returns whether it fired, and retryNow() self-gates correctly — if (fallbackPending) return false and if (cooldownTimer === null && !startupExhausted) return false — so calling it unconditionally is safe (returns false with no side effect when nothing is pending) and a manual resume now actually restarts work immediately instead of returning "active" while waiting for the original timer.

P2 — re-fetch task at spawn-commit (task-agent-manager.ts:908-916) — confirmed correct.
spawnWorkflowNodeAgentForExecution now does const freshTask = this.config.taskRepo.getTask(task.id) ?? task; validateTaskAllowsSpawn(freshTask); immediately before validating, so a parallel node that flipped the row to rate_limited after the caller loaded task can't slip a stale in_progress snapshot through. The ?? task fallback for a concurrently-deleted row is a harmless edge case.

Deferred to #812 (acceptable)

P2 — suppress terminal errors from speculative fallback switches (agent-session.ts:922): a ModelSwitchHandler internal switch failure broadcasts session.error (which the workflow error listener treats as terminal, marking the node blocked) before returning {success:false}, so even a subsequent successful fallback leaves the task blocked. Needs a non-terminal switch mode + error-listener coordination. Real but narrow (requires an internal switch failure, not the common unavailable-model path), recoverable, and tracked. Fine to defer.

CI / merge state on 2b612a7d6

Required checks all SUCCESS ("All Tests Pass"; 0 failures, no required check incomplete). Greptile Review (a non-required AI reviewer) was still finishing at review time → mergeState UNSTABLE (not failed/blocked). PR base dev, not draft, 80 review conversations all resolved (0 open).

Verdict

The round-7 P1 was a worth-the-catch feature-correctness gap (billing classification swallowed the primary resettable-cap case), now fixed with a tested pure helper and a tight, false-positive-free reset carve-out. Both P2s verify and improve behavior. All P0–P2 findings are fixed-and-verified or acceptably deferred; remaining work is the bounded task-lifecycle follow-ups in #811/#812. Approving — convergence is the right call.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b612a7d65

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE (own-PR, so posted as COMMENT — this is an explicit approval verdict).

Round-14 verification (first-hand, head 353c505a4) — greptile cleanup round

A cleanup round addressing greptile's 4 non-blocking P2 style/edge comments. All verified:

1. classifyLimitKind CJK dedup (fallback-recovery.ts:343-351) — correct.
The removed raw-message CJK pass was provably redundant: String.toLowerCase() is a no-op on CJK ideographs (no case mapping), so the single lowercased pass (lower.includes(kw.toLowerCase())) is byte-identical to the raw pass for every CJK keyword, and still handles ASCII keywords via the lowercasing. No behavior change.

2. "deferrive" → "deferred" typo (rate-limit-watchdog.ts, 2 log strings) — trivial.

3. LOCAL_DATETIME_RE regex hardening (fallback-recovery.ts:148-164) — verified empirically.
The regex now accepts a bare fractional LOCAL datetime (17:55:10.123) via (\.\d+)? while still rejecting zoned timestamps, and closes a partial-fraction backtracking hole by adding a bare \d to the negative lookahead. I ran the regex against 7 cases directly:

• MATCH (correct): 17:55:10.123, 17:55:10, 17:55:10.123 重置, trailing sentence period.
• NO-match (correct): 11:00:00.000+08:00 (the backtracking hole — confirmed closed), 11:00:00.000Z, 11:00:00+08:00.
  Local test shard: 47/47 pass (incl. the new bare-fractional-LOCAL test). The mechanism: greedy (\.\d+)? consumes the full fraction then the [+-]\d{2}/Z lookahead rejects a zone; every backtracking shorten-path now trips the added \d (or \.\d) lookahead, so a zoned timestamp can't be partially rescued as local.

4. shouldStopWorkflowForStatus blocked-scope restriction (space-task-handlers.ts:460-482) — correct.
The blocked teardown target is now restricted to rate/usage-limited origins (toBlockedFromPaused), so rate_limited/usage_limited → blocked still tears down the armed cooldown session (the earlier "stop the cooldown when manually blocking a limited task" fix is preserved), while in_progress → blocked no longer triggers teardown — keeping this PR's blast radius scoped to the rate-limit work and leaving the pre-existing in_progress → blocked behavior unchanged. toStopped (open/cancelled) teardown from any active/paused origin is unchanged.

Codex P2 (acknowledged, not a regression)

recoverWorkflowBackedTask commits in_progress before the retry outcome — this is the previously-deferred #812 item (the "await a successful retry before restoring the task" finding), acknowledged in-thread and consolidated into #812. Not a new regression introduced this round.

CI / merge state on 353c505a4

Required checks all SUCCESS (0 failures; local test shard 47/47). Greptile Review (non-required AI reviewer) still finishing → mergeState UNSTABLE (not failed/blocked). PR base dev, not draft, 85 review conversations all resolved (0 open).

Verdict

Clean, low-risk cleanup round; every change verified first-hand (regex empirically + local tests). Two independent reviewers (codex across 7 rounds, greptile 4/5 merge-ready) now align on merge-readiness, with all P1s fixed and remaining work consolidated into the bounded #811/#812 task-lifecycle follow-ups. Approving — converge at the next fully-settled CI.