# PR #22775 review, round 1 (claude)

Reviewed head 34a6a9635aa2d3e764a0650149e8a222ebd7fe9f (local, unpushed; the forge still shows 8f9294071b) against merge-base 21da3061aa. I read the full diff (5 files), the issue (#16815), the PR description, `implement.md`, and the evidence logs.

## 1. Value, shape, product decisions

The PR is worth having. The incident is real and specific: a run fails on a usage limit, then completed background-task wake runs make the limit disappear, so recovery is never armed. The basic approach is right, and the change stays small. It makes one shared selection rule in `usageLimitBlockedRun`, mirrors it in both SQL lookups, and widens three filtered command reads. There is no schema change and no new service.

Product decision: the rule is broader than the incident. Any *completed* run whose first root input is a notification is skipped, permanently, including a wake that ran **after the reset** and did real work. The PR title says "empty wake runs", but the code does not check for empty. In practice:
- With auto-resume off (the default), after a productive post-reset wake the thread still shows `failed` / `usage_limit`. It stays that way until the user sends a message.
- If recovery is armed but not yet delivered, a "Continue where you left off." message still fires after the wake already did its work.

A smaller, more precise rule: skip a completed notification wake only if it **completed at or before the limited failure's `resetAt`**. A wake inside the limit window cannot have done provider work. That covers every row of the incident table (all wakes ended between t0+2 s and t0+5 m, against R = t0+4 h 20 m). Its cost: SQL and the helper must also read the reset time. Maintainers should decide between "any completed wake" and "wake inside the window". I recommend the window rule, but it is not a blocker by itself.

## 2. Confirmed bugs

### B1 (medium, blocks): Stop after a completed wake no longer prevents automatic recovery
- `apps/server/src/orchestration-v2/Orchestrator.ts:9128`: `holdStoppedThread` seeds a disarmed `limitRecovery` (`autoResume: false`) only when `latestExecutedRun(runs)` is the failed limited run.
- `Orchestrator.ts:10099`: the Stop marker uses the same unfiltered `latestExecutedRun`.
- After this PR, a completed notification wake is still the "latest executed run" in those two places. But it is *skipped* by the recovery candidate SQL (`ProjectionStore.ts:3794`) and by `usageLimitBlockedRun`.
- Failing sequence:
  1. Auto-resume is on, or the user enables it later.
  2. A run fails with `usage_limit` and a future reset.
  3. A background wake completes.
  4. The user presses Stop before the sweep arms recovery. This is the same window the existing `stop-before-arm` case covers.
  5. `holdStoppedThread` sees the completed wake, so it does not seed recovery, and `limitRecovery` stays null.
  6. `getLimitRecoveryCandidates` now returns the failed run. `limitRecoveryCommand` takes its mismatched-recovery branch and arms `autoResume: true`.
  7. At reset, the thread receives "Continue where you left off." even though the user stopped it.
- Variant: stop with auto-resume off, then enable auto-resume before the reset. Same result.
- On main this path could not resume at all, because the wake hid the limit. Without a wake, the existing `stop-before-arm` invariant ("no candidate after stop") holds. So the PR reopens that invariant only on its new path.
- Affected: every provider (the logic is provider-independent). The trigger needs a background-task wake after a limit. In practice that means Claude background subagents or tasks, as in the issue.
- Evidence: traced through the code, not executed. I made no edits, per the brief.
- Fix: put the "skip completed notification wakes" selection in one exported helper, for example `latestLimitRelevantRun(runs, turnItems)`, and call it from `holdStoppedThread` and the stop marker. `holdStoppedThread` currently loads only `runs`, so it also needs `user_message` and `notification` turn items for root nodes.
- Test: add a `completed-wake-stop-before-arm` case to the existing table. Completed wake, then `thread.stop`, then assert there is no recovery candidate for `autoResume: true`.

### B2 (low-medium, fix with B1): the usage-limit banner has no Resume action after a completed wake
- `apps/web/src/components/ChatView.tsx:1026` computes `resumableRunId` from the unfiltered `latestExecutedRun`. The server guard at `Orchestrator.ts:4801` checks `latestExecutedRun(projection.runs)?.id !== source.id` the same way.
- After this PR, `deriveThreadRuntime` (shared presentation) shows the thread as `failed` / `usage_limit` because of the limited run. But `resumableRunId` is the completed wake, so `canResume` is false. And if a client did send `manualContinuationOfRunId` for the limited run, the server would reject it with "This thread can no longer be resumed from that run."
- On main, the thread showed `completed`. That was consistently wrong (no limit shown, no Resume), not a capability this PR removes. But the PR now shows a limit error the user cannot act on from the Resume control. With default settings (auto-resume off) this is the path most users take. Affects web and desktop. Mobile has no `manualContinuationOfRunId` path.
- Fix: the same shared helper in `ChatView` `resumableRunId` and in the server manual-continuation guard.

## 3. Sensitive surfaces

No auth, credentials, secrets, permissions, approvals, runtime or interaction modes, sandbox, trust, or MCP code changed. The pending runtime-request guards are unchanged. Stop semantics (B1) are a correctness issue, not one of the listed surfaces. Not NEEDS_JULIUS.

## 4. Regex, parser, and loop complexity

- No regex or parser was added or changed. There are no nested or overlapping quantifiers to block. Message text is never inspected.
- `usageLimitBlockedRun`: one pass over turn items into a Map, then a filter and a linear pass over runs. That is O(I+R) time and O(I+R) space. It is called in command validation and shell presentation, which already scan the same arrays.
- With 3000-token malformed strings (`complexity.log`): 1 wake took 0.0009 ms median and 1.09 ms max; 3000 wakes/items took 0.24 ms median and 0.51 ms max. Text is opaque, so string size does not change the cost.
- SQL: the new correlated probe runs once per completed candidate run. `EXPLAIN QUERY PLAN` on a read-only open of real data uses `turn_items_node_ordinal_idx (node_id=?)`, so it is an index seek and stops at the first input.
  - Measured on the real database (154 threads, 356 runs, 10.8k items), blocked-run subquery alone: 1.5 ms → 3.4 ms median. That is about 5 µs per completed run, and the cost grows with total runs on full shell snapshots.
  - The per-thread shell path is bounded by that thread's runs. The recovery sweep is pre-filtered to threads with a usage-limit failure.
  - Acceptable, but the full-snapshot cost now grows with every completed run in the environment.
- Real data: the new rule changed the blocked-run pick for 3 threads. All 3 new picks are `completed`, so nothing visible changed.

## 5. Evidence

- Baseline (`baseline-tests.log`): both new cases fail on the parent commit with "candidate missing" and "missing when due". That matches the issue.
- I reran from `/home/ubuntu/wt/issue-16815`, confirmed to be at the same SHA: `vp test run runtimeLayer.test.ts threadExecution.test.ts` passed 96/96.
- Lint, format, and typecheck logs are from the implementer. I did not rerun them.
- Limitations:
  - No real Claude limit was used. The test wake is synthetic (`events.write` of a `run.updated` completed run), not a wake driven through the provider adapter.
  - The runtime transcript is pending the next phase. I don't count it as a code defect.
  - The tests cover only completed wakes inside the limit window. Nothing covers a productive wake after the reset (see §1) or Stop combined with a wake (B1).

## 6. Requested tests, each naming the failure it would catch

1. `completed-wake-stop-before-arm`: catches B1, where Stop after a completed wake still lets the sweep arm and auto-resume.
2. If the window rule is adopted: a completed notification wake *after* `resetAt` supersedes the limit (no candidate, status `completed`). Catches a duplicate "Continue" message and a stale limit banner after productive wake work.
3. (Optional, shared helper) A table case asserting that the manual-continuation guard accepts the limited run after a completed wake. Catches B2 on the server side.

## 7. Compatibility

- No schema, migration, or contract change. Existing data is read under the new rule only. Threads already sitting in the incident state will become armable on upgrade if their reset is still in the future and their limit failure is newer than the last completion. That is the intended effect.
- Older clients receive shell data computed by the server and keep their own `deriveThreadRuntime`, so an old client may still show `completed` for these threads until it updates. That is harmless.
- Reconnect: no new state; snapshots are recomputed. Cancel and Stop: see B1.
- Platform: shared code in `packages/shared` and `client-runtime` covers web, desktop, and mobile. Mobile has no Resume control here.
- Every provider adapter is affected equally, since only orchestration changed.
- The widened `turnItemTypes` in `readCommandProjection` adds root notification and user_message items to every command's write-transaction read. `user_message` was already included, so the only addition is notifications, which is a small cost.

## Minor notes
- The PR title and body say "empty wake runs", but the rule is "any completed notification-first run". Align the wording, or adopt the window rule.
- The comments `-- usageLimitBlockedRun: completed notification wakes preserve the limit.` sit after the new clause, where the old `latestExecutedRun` comment was. Fine.
- The test for `completed-wake-*` reads candidates directly instead of the shell snapshot, so the shell path for those cases is only covered by the client-runtime test.

VERDICT: FIX_FIRST. B1 reintroduces the "Stop must not auto-resume" invariant violation on the new completed-wake path. B2 and the resume guard should share the same selection helper. Maintainers should also decide between "any completed wake" and "wake inside the limit window".