# PR #22775 review, round 2 (claude)

Reviewed head [80d27e96db](https://git.cafe/pingdotgg/t3code/commit/80d27e96db888d3ab0dbf12314f136f4e8b3e9b4) against merge-base [21da3061aa](https://git.cafe/pingdotgg/t3code/commit/21da3061aa006fc852ea4bd84c31ec084a3afa26). I read the full six-file diff in the automatic worktree (detached at the head SHA), plus both round-1 reviews and `implement-followup.md`. I followed the changed callers: `usageLimitHoldsQueue`, `startNextQueuedRun`, `threadShellFromProjection`, the shell blocked-run join, `limitRecoveryCommand`, `markStoppedRun`/`stopReachedRun`, `dispatchStop`, and the ChatView Resume wiring.

## Confirmed findings

None. Each round-1 finding is fixed, and the fresh pass found no new defect.

### Round-1 findings re-checked

- **B1 (Stop-before-arm after a completed wake): fixed.** `holdStoppedThread` (`apps/server/src/orchestration-v2/Orchestrator.ts:9131-9156`) now chooses `usageLimitBlockedRun(...) ?? latestExecutedRun(...)`. It loads root input and error items for the whole thread.
  - It seeds a disarmed `limitRecovery` with `{runId: limited, resetAt, autoResume:false, requestId: stop}`.
  - `limitRecoveryCommand` then matches runId and resetAt, sees `!recovery.autoResume`, and returns null (`UsageLimitRecoveryWorker.ts:36-55`). That holds even if the user enables auto-resume later.
  - It now marks the hidden limited source as stopped. This matches the no-wake case, where `dispatchStop` already marked that run because it was `latestExecutedRun`.
  - The marker does not gate manual or automatic usage-limit continuation. `stopReachedRun` is consulted only for restart continuations, agent PR watches, subagents and interrupts. So "Stop, then manual Resume" behaves as it does without a wake.
  - Covered by `completed-wake-stop-before-arm`.
- **B2 / codex P2 (Resume selector vs server guard): fixed and aligned.**
  - Client: `ChatView.tsx:1027-1029` selects `usageLimitBlockedRun ?? latestExecutedRun`.
  - Server: the guard at `Orchestrator.ts:4801-4804` uses `usageLimitBlockedRun` for failed sources and `latestExecutedRun` for interrupted ones.
  - For a failed usage-limit source with no wake, the two are equal, so the earlier behavior is unchanged.
  - When the latest executed run is interrupted, blocked is null and the code falls back correctly.
  - The `usage_limit_wake` manual-resume case drives the client expression into the real dispatch. It also checks that the guard rejects Resume while the wake is active.
- **Scope of "ignore wakes": narrowed as requested.** A completed notification-first root run is ignored only when `completedAt <= resetAt` (`packages/shared/src/orchestrationV2ThreadError.ts:83-108`).
  - Completion after reset supersedes the limit.
  - A missing or invalid reset gives `NaN`, so the wake supersedes, which is the old behavior.
  - Covered by `completed-wake-postreset`: no recovery candidate, shell `completed`, a stale automatic dispatch starts nothing, and a stale manual continuation is rejected.
- **Manual Resume releasing queue holds.** I accept the author's response. The `interrupted` and `usage_limit` cases already asserted that the queued run runs after the continuation. Unholding on manual return is existing behavior, not something this PR introduced.

### SQL / shared parity (the unresolved objection)

I checked these clause by clause in both SQL copies: recovery candidates `ProjectionStore.ts:3786-3830` and shell blocked run `ProjectionStore.ts:5839-5885`.

- **Latest failed run.** `ORDER BY completed_at IS NULL DESC, completed_at DESC, ordinal DESC` mirrors `latestExecutedRun(failed runs)`/`runRanAfter`, where a null end counts as infinity. Ordinal is unique per thread (`runs_thread_ordinal_idx`).
- **Latest root error.** `updated_at DESC, ordinal DESC, id DESC` mirrors `latestRootProviderFailure`.
- **Class gate.** For a non-usage-limit failure, `COALESCE(class='usage_limit' AND …, 0)` evaluates to 0, so no wake is skipped. The outer `r.status='failed'`/item-class filters (or `blocksQueue` in the shell) then reject it, the same as the early `return null` in TS.
- **Null `resetAt` or `completedAt`.** `julianday(NULL)` makes the comparison NULL, which COALESCE turns into 0, so the wake is kept. This matches `!(x <= NaN)` and the TS `completedAt === null` guard.
- **Null `rootNodeId`.** The `wake.node_id IS NOT NULL` guard makes the subquery NULL, and `NULL IS 'notification'` is false, so the run is kept. This matches the TS `rootNodeId === null`.
- **First input.** The first `user_message`/`notification` by ordinal on the root node matches the TS map. TS keys by nodeId across runs and SQL by run plus node, but root nodes are per-run.
- **Shell vs in-memory replay.** `threadShellFromProjection` reaches `usageLimitRunPresentedAsLatest` through the shared helper, so it stays in step with the SQL shell join.

Two minor divergences are only theoretical:

- `Date.parse` accepts non-ISO or zone-less strings that `julianday` rejects or reads as UTC. Both adapters emit `DateTime.formatIso` (Z) values, so this does not occur today.
- A bounded client projection could drop a wake's root notification item. Wakes are the newest rows, so they stay inside the window.

## Minor notes (non-blocking)

1. **Every command now reads notification items.** `readCommandProjection` runs inside the write transaction for every command, and it now also decodes every notification turn item (`Orchestrator.ts:1043`).
   - A read-only count on the live database: 249 notification items, all root-node, 391 KB in total. The heaviest thread has 91 items, about 136 KB per command read.
   - That is the same order as the `user_message` items the read already loaded (162 items, 735 KB). The cost is acceptable, but it grows with background-task-heavy threads.
   - `holdStoppedThread` likewise reads every root input for the thread, but only once per Stop.
   - No action is needed now. If it shows up in profiles, a root-node filter would bound it.
2. **Product rule, for awareness.** Before reset, even a wake that did real work is ignored. One example is a pooled-account hub that routes the wake to another account. In that case the limit banner stays, `usageLimitHoldsQueue` keeps unheld queued messages waiting, and armed auto-resume still sends "Continue where you left off." at reset. This is the decision the spec asked for (incident wakes precede reset), and it is the correct default for real quota windows. It is worth one line in the PR body.
3. **Test coverage.** `completed-wake-before-arm` and `completed-wake-stop-before-arm` check recovery through `getLimitRecoveryCandidates`. The shell SQL join is checked only in the post-reset case. Both copies share the same clause, so the risk is low.
4. **PR wording.** The title and body should describe "notification wakes completed before the reset", not "empty wake runs".

## Sensitive surfaces

This PR does not change any of the following: auth, credentials, permissions, approvals, runtime or interaction modes, sandbox, trust, or MCP. Pending runtime-request guards still apply to arming, manual continuation and automatic continuation. Stop now marks one more run as stopped, which matches the existing no-wake semantics. Not NEEDS_JULIUS.

## Complexity (actual head)

- **Shared helper.** It filters failed runs, scans for the latest root failure, builds a first-input Map in one pass, then filters and selects runs: O(R + I) time and space. No regex, no parser, and no text is inspected. `Date.parse` runs once on `resetAt`.
- **3000-token malformed input.** I reran the provided `followup-complexity.mjs` against `/home/ubuntu/wt/issue-16815` after confirming HEAD = 80d27e96db (`review-2-claude-complexity.log`, 1000 calls after warmup):

  | Case | Median | p95 | Max |
  |---|---|---|---|
  | 1 wake | 0.0013 ms | 0.0045 ms | 0.66 ms |
  | 3000 wakes/items | 0.256 ms | 0.365 ms | 0.47 ms |
  | Malformed 3000-token `resetAt` | 0.117 ms | 0.132 ms | 0.26 ms |

  Cost is linear and does not depend on string size.
- **SQL.** It adds two ordered LIMIT 1 lookups per thread: `runs_thread_status_idx` for failed runs, and `run_ordinal_idx` for the error. The per-candidate first-input probe (`node_ordinal_idx`) now sits behind the reset gate, so it is evaluated only for completed candidates inside a usage-limit window. The recovery sweep stays prefiltered to threads with a usage-limit error.

## Evidence

- `review-2-claude-tests.log`: `vp test run runtimeLayer.test.ts threadExecution.test.ts` at the exact head gives **99/99 passed** (2 files).
- `review-2-claude-complexity.log`: the timings above.
- I read the implementer's baseline log: the four new failures occur on the starting commit, and the 19 original recovery cases pass. I did not rerun scoped typecheck, lint or format; the implementer's logs report exit 0.
- Not run, as the brief scoped it:
  - A live provider or runtime transcript. That is the lead's planned stage and not a code defect.
  - A browser check. The ChatView change is a selector expression, already exercised through the server regression.
- The wake is synthetic (`events.write`), not driven through an adapter.

## Tests

The tests needed for the round-1 failures are now present. Optional additions:

- A shell-snapshot assertion (`getShellSnapshot` `status === "failed"`, `lastErrorClass === "usage_limit"`) in `completed-wake-before-arm`. It would catch drift between the shell blocked-run SQL and the candidate SQL.
- A shared-helper case with a wake that completed before reset, followed by a new user-message run that completed before reset (for example after a provider switch). It would catch a regression that skips user-initiated work.

## Compatibility

- No schema, migration or contract change. Existing incident threads become armable on upgrade only if their reset is still in the future.
- Older clients keep the old shared helper until they update. They may show `completed` and offer no Resume for these threads. That is harmless, and the server guard still accepts the limited source from new clients.
- The change covers all providers and local, remote and tunnel connections equally.
- Mobile has no manual-continuation caller, and shared presentation covers it.

Disk: 12 GB free. No edits, pushes, comments or installs. My only artifacts are the three `review-2-claude*` files in the evidence directory. I left the automatic worktree detached at the head for the lead to clean up.

VERDICT: MERGE