fix(t3x): stop a user message destroying a pending auto-resume - #80
fix(t3x): stop a user message destroying a pending auto-resume#80radroid wants to merge 1 commit into
Conversation
Closes #39. Two defects compounded into a permanently stranded thread: a usage limit armed a resume, the user typed "keep going" at the banner, that message was itself rejected so it started nothing, and the wake tick then threw the arm away as `user-took-over`. Net pending afterwards: zero. Measured on the reporting install at 4 of 17 armed resumes (~24%) since #6. 1. `guards.ts` — drop the `user-took-over` branch. It is the same negative-evidence mistake #6 fixed on the line below it: "a message exists that wasn't there when we armed" is not evidence the human took the wheel, and in practice it is the opposite signal. Everything the branch reached for is still covered — `progressing` (actively driving), `awaiting-input` (blocked on a prompt), `thread-advanced` (a different live turn), and the per-thread switch for "stop entirely". `baseline.newestUserMessageId` stays: it is part of the persisted record, it is re-captured on every (re)schedule, and it is what makes a stranded arm diagnosable from the state file. 2. `decide.ts` — narrow `already-pending`. A rejection naming a CONCRETE reset time later than the pending one now supersedes it instead of being dropped, so a `seven_day` limit landing on an armed `five_hour` no longer leaves the arm firing into a window that is still shut. Restricted to `windowOpensInFuture` on purpose: a ladder-derived time is `nowMs + delay`, so it is later on every telemetry re-emit, and letting those supersede would push the arm out forever and flood the timeline. An earlier reset never supersedes either — the existing arm is already the conservative choice. A supersede re-writes the record with a fresh `captureBaseline`, so it is also the re-arm path, and posts a distinct `t3x.auto-resume.rescheduled` note rather than a second "scheduled". The issue's third item — detecting the org monthly spend limit — is a genuinely separate feature (no `account.rate-limits.updated` path reaches it at all) and is deliberately not in scope here, per the triage. Tests: 4 new behavioural cases, each verified to fail against the old code — guards (a newer user message does not cancel; it still cancels when that message is actually being worked on), decide (supersede, earlier-window no-op, ladder-churn guard), and two reactor integration tests covering the reported shape end to end. Reactor suite run 12x for flake. `docs/t3x/loop/DESIGN.md` §6 gets a dated note: guard #9 still stands, but the hazard it was defending against is gone.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Runtime verification — A/B on a real thread, not just unit testsRan this end to end in an isolated dev environment (worktree-local Identical seeded With this PR's code — fires: Claude genuinely picked the work back up — the thread had been counting to 40, and the resumed turn continued from 41. With the Same fixture, same thread, nothing dispatched, attempt not even counted. That is the ~24% loss the issue measured, reproduced on demand and then fixed.
Two side observations from the same session1. 2.
Observed directly: after the first turn completed, This does not change the correctness of either guard — #6's "require positive evidence" rule is right whether the column clears or not, and the existing regression test still pins the null case defensively. But the stated mechanism in that comment does not match this build, and since that comment is the documentation for why the guard looks the way it does, it is worth a proper look. Not fixing it in this PR; flagging it so the next person to read that comment does not trust it blindly. |
Closes #39.
The bug
A usage limit arms a resume. The user types "keep going through the night" at the banner. That message is itself rejected by a limit, so it starts nothing — and the wake tick then throws the arm away as
user-took-over. Pending count afterwards: zero. The thread never wakes.Measured on the reporting install: 4 of 17 armed resumes (~24%) lost this way since #6 shipped.
The fix
1.
guards.ts— drop theuser-took-overbranch.It is the same negative-evidence mistake #6 fixed on the line directly below it. "A message exists that wasn't there when we armed" is not evidence the human took the wheel; in practice it is the opposite signal, because the banner is exactly when someone leaves instructions and steps away.
Everything the branch was reaching for is still covered:
progressingawaiting-inputthread-advancedfireOnebaseline.newestUserMessageIdstays in the shape: it is part of the persisted record, it is re-captured on every (re)schedule, and it is what makes a stranded arm diagnosable fromt3x-auto-resume.json.2.
decide.ts— narrowalready-pending.A rejection naming a concrete reset time later than the pending one now supersedes it instead of being dropped, so a
seven_daylimit landing on top of an armedfive_hourno longer leaves the arm firing into a window that is still shut.Deliberately restricted to
windowOpensInFuture. A ladder-derived time isnowMs + delay, so it is later on every telemetry re-emit — letting those supersede would push the arm out forever and flood the timeline with reschedule notes. An earlier reset time never supersedes either: the existing arm is already the conservative choice. Both are pinned by tests.A supersede rewrites the record with a fresh
captureBaseline, so it is also the re-arm path, and posts a distinctt3x.auto-resume.reschedulednote rather than a second "scheduled".Scope
The issue's third item — detecting the org monthly spend limit — is not in this PR, per the triage on the issue: spend-limit detection does not exist anywhere in the repo (no
account.rate-limits.updatedpath reaches it) and a monthly cap probably should not ride the backoff ladder at all. Worth its own issue. This PR is the two changes that stop the loss.Verification
vp test run apps/server/src/t3x/autoResume— 76 passed (8 files).settleUntil/advanceUntilhelpers rather than fixed spins.vp lint apps/server/src/t3x/autoResume— clean.tsgo --noEmit -p apps/server— 0 errors.Seam cost
None. Every changed source file is fork-owned under
apps/server/src/t3x/autoResume/;git log <merge-base>..upstream/main -- apps/server/src/t3x/is empty. Nodocs/t3x/SEAMS.mdrow changes.docs/t3x/loop/DESIGN.md§6 gets a dated note: guard #9 still stands (nudging a thread inside a usage-limit window is pointless), but the hazard it was the last line of defence against is gone.