Reactivate threads when a provider resumes work on a completed turn - #1697
Reactivate threads when a provider resumes work on a completed turn#1697SawyerHood wants to merge 2 commits into
Conversation
Codex can keep streaming root work (commands, file changes, messages) on a turn it already reported as completed, and then complete that turn a second time. BB only tracked status from turn/started and turn/completed, so the thread showed idle while commands ran, and thread/stop had no active turn to interrupt. - Server: item/started on an idle thread for a completed turn, with no stop after that completion, applies run.started again. - Daemon: RuntimeTurnState reopens the active turn from that work so stop/steer target it, and the provider session is not reaped as idle. Fixes #1646 Co-Authored-By: Claude <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and test coverage. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| and( | ||
| eq(storedEvents.threadId, args.threadId), | ||
| eq(storedEvents.type, "system/thread/interrupted"), | ||
| gt(storedEvents.sequence, turnCompleted.sequence), |
There was a problem hiding this comment.
🚨 slopcop/review — A later completion can bypass a prior stop.
resolveReopenedTurnId uses the latest turn/completed as the lower sequence bound. The order turn/completed → stop → later turn/completed → root item/started moves that bound past the stop. The thread then returns to active after the user stopped it. A temporary route test reproduced active instead of idle. Please compare the stop with the turn start or request boundary. Please add this event order as a regression test.
There was a problem hiding this comment.
Confirmed and fixed in 2ef9350: the reopen guard now checks for any stop after the turn request that produced the turn (same lower bound as hasThreadStopBeforeTurnStarted), with a regression test for completed → stop → completed → item/started.
| // that work so stop/steer can still target it. | ||
| if ( | ||
| event.type === "item/started" && | ||
| event.item.type !== "userMessage" && |
There was a problem hiding this comment.
🚨 slopcop/review — A background task can reopen and strand the turn.
This predicate excludes user messages and parented items. It still accepts an unparented backgroundTask. The runtime permits background tasks to outlive a turn while the thread stays idle. Their item/backgroundTask/completed event does not clear this active-turn state. A temporary unit test reproduced turn-1 instead of null. Please exclude background tasks in the server and runtime predicates. A shared domain helper can keep both rules equal.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This change makes a completed thread active again when its provider sends more work. It also restores the active turn for stop and steer actions.
I found two high-impact correctness defects. Both defects can leave the thread active when it should stay idle.
- A later completion can move the stop boundary past an earlier user stop. The next root item then reactivates the stopped thread.
- An unparented background task can reopen a completed turn. Its background completion never closes that reopened turn.
The first defect also affects the user-control security boundary. I found no host ownership or authentication defect.
The new database queries use targeted indexes. The runtime additions use constant-time map operations. I found no performance defect.
The server and runtime duplicate the root foreground-item rule. A shared domain helper would prevent the two rules from drifting.
Validation:
- All GitHub checks passed.
- The clean server route test passed all 12 tests.
- The clean runtime state test passed all 12 tests.
- A new stop-order test failed with
activeinstead ofidle. - A new background-task test failed with
turn-1instead ofnull. git diff --checkpassed.
A browser test does not reach this daemon-only event route. The route and runtime tests exercise the changed behavior directly.
I posted both defects as inline comments. This is a comment-only review, as required.
- The reopen guard now checks for any stop after the turn request that produced the turn, so a later completion cannot move the boundary past a user stop. - Server and daemon share isTurnReopeningWorkItem, which excludes user messages, delegated child items, and background tasks that can outlive a turn. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Fixes #1646
Problem
Codex can keep streaming root work (
commandExecution,fileChange,agentMessage,reasoning) on a turn it already reported withturn/completed, then complete that turn a second time. The store proves this: BB rejects turn-scoped content for a turn with no storedturn/started(409), and the idle-gap items in the report were persisted, so they used an already-started turn id. BB only derived thread status fromturn/startedandturn/completed, so:idle(no spinner,bb thread wait --status idlereturned early) while commands ran;thread/stopwas a no-op and idle-session reaping could target the busy provider session.Change
apps/server/src/internal/events.ts): a rootitem/startedon an idle thread for a turn that already has a storedturn/completed, with nosystem/thread/interruptedafter that completion, appliesrun.startedagain. The nextturn/completedsettles the thread toidleas before.packages/agent-runtime/src/runtime-turn-state.ts,runtime.ts):RuntimeTurnStatereopens the active turn from that work so stop/steer target it, and the provider session is marked busy again.No wire format change; no
HOST_DAEMON_PROTOCOL_VERSIONbump.Tests
apps/server/test/internal/internal-event-append-ownership.test.ts: reactivation on late provider work; stopped thread stays idle.packages/agent-runtime/src/runtime-turn-state.test.ts: reopen from root work; ignore delegated child work and active turns.pnpm exec turbo run typecheck/test --filter=@bb/agent-runtime --filter=@bb/serverpass (one pre-existing unrelated failure ininternal-skill-trees.test.tsfrom a local umask file mode).