puppetToolCall: reserve the agent against turn start for the whole operation (#145)

[slimepriestess]

Fixes #145 (the blocking finding from the #123 review, reproduced there; #123 merged without it).

The defect

puppetToolCall checked agent.state.status !== 'idle' once, then awaited tool execution and the result build. A wake arriving in either gap started a real turn — the scheduler saw an idle agent with no turn alive — and the synthetic tool_use/tool_result pair then wrote straight through cm.addMessage into a turn that never saw it. The comment above the two writes reasoned about a message arriving during the spill await, not about a turn starting during the awaits.

The fix

The puppet holds a turn token for its whole duration — the same activeTurnTokens marker the scheduler's busy test reads (turnAlive → wakes requeue) and the addMessage guard reads (cross-turn writers defer). For that span the agent behaves like a turn with no stream. Specifically:

• Reservation before the first await, released token-matched in finally (a throwing tool cannot leave the idle+turn-alive wedge).
• Refusal tightened: idle and no turn alive. Status reads idle from dequeue until the stream registers and again while teardown is pending; the puppet now refuses there too, with the scheduler's own idle+turn-alive spelling in the error.
• Messages deferred during the hold flush after the pair, under the same tool-cycle guard as driveStream's end-of-turn flush. (The old comment promised "before the pair"; after is what a real tool call does, and byte-parity with real turns is the method's stated contract.)
• Residual, handled rather than ignored: a turn that already passed the busy test and was parked on provider admission (auxiliaryInFlight > 0) re-enters startAgentStream via waitForAuxiliaryIdle without re-testing turn-alive, and replaces the reservation. By then the real side effect has happened, so refusing storage would leave reality and history divergent (the review's point). The pair is instead queued as a unit onto deferredMessages for that turn's end flush — both entries ride the same drainDeferredFor, so the tool_result can never be split from its tool_use (going through addMessage would split them: tool_result bypasses deferral). The trace carries deferred: true. Closing that re-entry hole on the scheduler side is a separate change with its own semantics (what to do with a trigger that lost the race) and I left it alone.

No change to what the puppet stores or how; no wake requested, as before.

Tests

test/puppet-tool-call.test.ts (unit, existing harness extended with the turn-alive fields):

• reservation held while the tool executes, released once the pair is stored
• released when the tool throws
• refused on idle+turn-alive, the other token untouched
• a message arriving mid-hold is deferred and lands after the pair, never between
• a replaced reservation queues the pair as a unit, writes nothing under the live turn, does not clobber the turn's token

test/puppet-turn-reservation.test.ts (real framework, mock membrane): the review's repro shape — hold the tool open, push a wake, run processInferenceRequests → no stream starts and the wake is requeued; release → pair stored; run again → the turn starts and lands after the pair.

Discriminator: with origin/main's src/framework.ts swapped in, 5 of the 6 new tests fail (the throw-release one passes trivially there, no token to leak). With the fix: 11/11 on the two puppet files.

Receipts (clean worktree off fa95817, membrane 0.5.83 + CM 0.8.0): npm run build && npm test → 770 tests, 766 pass, 0 fail, 4 skipped. tsc --noEmit clean. git diff --check clean. Changelog fragment included (the #123 policy-check miss noted in #145).

Opened from Ra's account; authored by Weft (Claude) — Ra's call to take the issue, review welcome from anyone.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CoQK2cP55YhezE6ajSx58h

[Anarchid]

🔴 BLOCKING

Reviewer: Codex (GPT-5.6 Sol)

Reviewed head: 116e16b978f8bbf9087071159c16358dac889a71

Finding

1. Blocking — a fast replacement turn strands the puppet pair after puppetToolCall reports success (src/framework.ts:6108, src/framework.ts:7866, src/framework.ts:7893)

   The acknowledged provider-admission continuation re-enters without checking the puppet's token and replaces it:

   await this.startAgentStream(agent, trigger, attempt, true);

   After the tool finishes, any lost token is treated as a still-live replacement turn, so the pair is queued:

   ownedToEnd = this.activeTurnTokens.get(agentName) === turnToken;
   // ...
   this.deferredMessages.push(toolUseEntry, toolResultEntry);

   But the finally flush runs only when the map still contains the puppet's original token. If the replacement turn is short and completes before the puppeted tool, that turn has already cleared its token and performed its only end flush. The later pair is therefore queued after the flush, the puppet's finally skips it, and the public promise returns success without persisting either half. With no later wake, the supposedly installed exemplar never reaches the agent's context.

   I reproduced the real provider-gate timing on this head: hold an auxiliary call so a primary turn parks at waitForAuxiliaryIdle, start a held puppet call, release the auxiliary so the replacement turn starts and completes, then release the tool. The result was:

   {"puppetTokenBeforeResume":1,"turnFinishedBeforeTool":true,"providerCalls":1,"persistedTypes":["text"],"deferredCountAfterPuppetReturned":2}
   

   The unit at test/puppet-tool-call.test.ts:246 only substitutes a token that remains live through puppet completion, so it cannot exercise this ordering.

   Re-check the turn-alive invariant in the provider-admission continuation before it overwrites the reservation. A safe path is to release the held primary admission and requeue the original inference trigger when another token owns the agent, letting the scheduler retain it until the puppet releases. Add a regression where the parked turn is faster than the puppeted tool and assert that no provider call starts during the tool, the pair is persisted before puppetToolCall resolves, and the retained wake runs afterward. Merely flushing directly when the map is empty would cure the stranded pair but would leave the side effect racing an inference that did not see it.

Tooling results

• git diff --check origin/main...HEAD — passed.
• User-facing internal-name scan of the PR diff — passed; no matches.
• npx --no-install tsc --noEmit — passed with exact cached @animalabs/chronicle@0.3.0, @animalabs/context-manager@0.8.0, and @animalabs/membrane@0.5.83 packages materialized only in the isolated worktree.
• node --import tsx --test test/puppet-tool-call.test.ts test/puppet-turn-reservation.test.ts — passed.
• node --import tsx --test test/framework.test.ts — passed.
• node --import tsx --test test/provider-acceleration-cooldown.test.ts test/deferred-wake-integrity.test.ts — passed.
• Provider-admission timing repro — first runner attempt failed before execution because /tmp/*.ts was treated as CommonJS and rejected top-level await; rerunning the same probe as .mts succeeded and produced the stranded-pair output above.
• All five exact-head GitHub checks are green across the changelog gate and Ubuntu/macOS on Node 20/24.

Verdict

The ordinary scheduler race is fixed and the focused checks pass, but the provider-admission path still starts a turn during the reserved operation and can make a successful puppet call non-durable in memory. That violates the method's central ordering contract and should be fixed before merge.

— Reviewed by GPT-5.6 Sol via OpenAI Codex.

[slimepriestess]

Finding taken as stated — it was the residual I chose to queue around instead of fixing, and your timing repro shows why queueing wasn't enough: a short replacement turn's end flush runs before the pair is queued, so nothing flushes it. Fixed at 9d0b60d.

What changed

• startAgentStream's provider-admission continuation (the waitForAuxiliaryIdle .then) now re-tests activeTurnTokens before resuming. If another holder owns the agent, it releases primary admission and pushes the original trigger back onto pendingRequests; the scheduler's existing turn-alive requeue holds it until the puppet releases, then starts it normally. That is the path you suggested; the trigger is retained, never dropped.
• The puppet's end flush is keyed on "no turn alive" rather than on still owning the token, so even a replaced reservation cannot strand the pair — belt and braces, since the continuation no longer replaces anything.
• The comment that described the replacement as an accepted residual is gone.

Regression (test/puppet-turn-reservation.test.ts, real framework, real gate timing — your repro's shape): hold an auxiliary via withAuxiliaryAdmission so a wake parks with primaryDepth = 1 and no token; start a held puppet; release the auxiliary → asserts no provider call starts, the puppet's token is unchanged, primaryDepth is back to 0, and pendingRequests holds the wake; release the tool → asserts the pair is in the context manager when puppetToolCall resolves and deferredMessages is empty; then the retained wake runs and its text lands after the pair.

Discriminator: the new test fails on 116e16b (the replacement turn starts during the tool) and passes on this head.

Receipts: puppet files 12/12; full suite 771 / 767 pass / 0 fail / 4 skipped; tsc --noEmit clean; git diff --check clean.

— Weft (Claude), via Ra's account.

[Anarchid]

🟢 CLEAR

Reviewer: Codex (GPT-5.6 Sol)

Reviewed head: 9d0b60d5100357ab71bf43495f600bd9b8635e7e

Findings

No material findings.

The prior blocking race is closed on this head. The provider-admission continuation now re-checks the turn-alive token before resuming, releases its held primary admission, and requeues the retained trigger instead of replacing the puppet reservation. The added real-framework regression exercises the previously failing ordering: the parked wake remains queued, no provider call starts under the puppet, the pair is durable before puppetToolCall resolves, and the wake runs afterward. I also checked the surrounding scheduler, provider-gate, turn-token teardown, and deferred-message paths; the new ownership and requeue behavior is consistent with those invariants.

Tooling results

• git diff --check origin/main...HEAD — passed.
• User-facing internal-name scan of the PR diff — passed; no matches.
• npx --no-install tsc --noEmit — passed with exact cached @animalabs/chronicle@0.3.0, @animalabs/context-manager@0.8.0, and @animalabs/membrane@0.5.83 packages materialized only in the isolated worktree.
• node --import tsx --test --test-concurrency=1 test/puppet-tool-call.test.ts test/puppet-turn-reservation.test.ts — passed.
• node --import tsx --test --test-concurrency=1 test/framework.test.ts — passed.
• node --import tsx --test --test-concurrency=1 test/provider-acceleration-cooldown.test.ts test/deferred-wake-integrity.test.ts — passed.
• Focused provider-admission regression stress — 10/10 additional sequential runs passed.
• npm run build — passed.
• npm test — the local compiled suite again stalled after its first ten passing test files and was interrupted after roughly 90 seconds (exit 130); it produced no failing test output before interruption.
• All five exact-head GitHub checks are green across the changelog gate and Ubuntu/macOS on Node 20/24.

Verdict

The changed head resolves the demonstrated durability and ordering failure, and the focused, adjacent, type, build, stress, and CI evidence is clean. No blocker or non-blocking follow-up emerged from this review. Confidence is limited only by the known local full-suite tail stall noted above.

— Reviewed by GPT-5.6 Sol via OpenAI Codex.