Repository navigation
fix(ts): ignore pending tool execution when interrupt state is not activated - #5628
Conversation
|
@strandly-the-agent can you review? |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Approve Clean, well-scoped bug fix. Centralizing the Review notes
Nice work on the parity reasoning and the extended-context writeup in the description. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Changes requested. The activated gate itself is correct and genuinely well-tested — I mutation-tested all three gates and each is caught by exactly the expected test, including the Python one. But it only closes the plain-prompt replay. Because activate() still runs after the InterruptEvent yields (agent.ts:2061 via _createInterruptResult, called at agent.ts:1975), a consumer that breaks the stream at InterruptEvent — the HITL pattern this PR is about — is left with activated=false plus a stored pending execution, and now:
- the tool call the human just approved is silently dropped on resume — base ran it, this PR answers the user without it,
endTurn, no error; - the stale
pendingToolExecutionis never cleared, and the next interrupt that activates without a tool cycle of its own replays it.
Moving this._interruptState.activate() above the yield loop at agent.ts:1968 fixes both and restores parity with Python, which activates at strands-py/src/strands/agent/agent.py:1804 before yielding its stop event. It fails exactly one test: the one this PR rewrote.
Measured 3-way matrix, verification, questions, appendix
Matrix — base df200532 / this PR 72fa875f / PR + activate() before the yields
All rows start by streaming until the first InterruptEvent and breaking (for await … if (event instanceof InterruptEvent) break).
| flow | base | this PR | PR + fix |
|---|---|---|---|
A. resume with response:'yes', tool-raised interrupt |
toolRuns=["tool-1"] |
toolRuns=[] |
toolRuns=["tool-1"] |
B. resume 'yes', BeforeToolCallEvent approval gate |
stop=interrupt, wedged |
stop=endTurn, toolRuns=[] |
stop=endTurn, toolRuns=["tool-1"] |
B. resume 'no', same gate |
stop=interrupt, wedged |
toolRuns=[] |
toolRuns=[] (denied) |
| C. plain prompt instead | replays tool-1 (the bug) |
model called ✅ but pending=true lingers |
TypeError: Agent is in an interrupted state… |
C. then an unrelated AgentStreamStage gate + resume |
n/a (already consumed) | replays tool-1, splicing assistant[toolUse]/user[toolResult] in three turns late |
n/a |
Base was unsafe but functional (it replayed unconditionally, so a 'no' would have run the tool too — exactly what this PR is fixing). This PR is safe but silently broken: resume() early-returns at interrupt.ts:284 because the state isn't activated, so the human's answer is never applied at all. Neither is right; the one-line move gives you both.
This is also where I'd push back on the automated assessment above: "the other direct reads … only run when the state is already activated, so they stay consistent" and "the gate doesn't regress legitimate in-memory resume" are both contradicted by rows A and B.
✅ Verified
- Head
72fa875fafa3c6b97de8ff72a24d997e106414ebvs basedf2005323b7720089ad0a8e990cb47f0ee604145. npx vitest run --project unit-node src/__tests__/interrupt.test.ts src/agent/__tests__/agent.interrupt.test.ts src/middleware/__tests__/ src/session/__tests__/ src/agent/__tests__/agent.snapshot.test.ts→ 12 files, all pass, no type errors.- Mutation tests (all three gates are load-bearing): removing the
activatedcheck ingetPendingExecutionfails 2 tests; removing it infromJSONfails 1; removing both Python guards atevent_loop.py:263,894fails exactlytest_event_loop_cycle_ignores_pending_tool_execution_when_interrupt_state_not_activated. So the Python parity test does exercise the event-loop guard — it isn't echoing a mock. - Python behaviour confirmed at runtime, not just read: after the same stream break,
activated: True,pending_tool_execution set: True, and a plain prompt is rejected withTypeError prompt_type=<class 'str'> | must resume from interrupt with list of interruptResponse's. - Candidate fix under the existing suites:
Tests 1 failed | 352 passed (353)— the one failure is this PR's rewritten test. I ran a targeted subset (interrupt / middleware / session / snapshot), not the full unit suite. - Artifacts:
evidence-matrix.txt,mutation-tests.log,repro-5628.test.ts,py-parity-repro.py,py-parity-repro.out.
Questions (non-blocking)
- Should the
fromJSONdrop-and-validate hardening land in Python too?strands-py/src/strands/interrupt.py:210-229restorespending_tool_executionunconditionally and has no equivalent validation, so the TS reader is now stricter than the Python one for the same wire format — which matters if a session is written by one SDK and read by the other. fromJSONsilently discards a pending execution whenactivated=false, whiletoJSON(interrupt.ts:359) still writes it whenever it is set — so a round-trip isn't idempotent. Intentional over alogger.debugon the drop?
Appendix — non-blocking (3)
agent.ts:1546: the new@throws SessionError if the snapshot's interrupt state carries a malformed pending tool executionis only accurate for an activated state; a non-activated one is dropped without validation, so no throw.- The
field=<…> |error prefix is consistent with repo convention, not new — seeinterrupt.ts:294,hooks/registry.ts:113,multiagent/graph.ts:609, andagent.py:1794on the Python side. Noting it only because it came up as a concern; no change wanted. - Other readers of
pendingToolExecutioncheck out:agent.ts:1381is short-circuited by theactivated &&conjunct;agent.ts:1850'scontinuecan't spin because_throwIfCancelled()runs at the top of each cycle (agent.ts:1663);1869and the writers at2597/executor.ts:254are unaffected.
72fa875 to
18bfe3e
Compare
|
Addendum to my review above — one measured alternative, in case you'd rather keep accepting a fresh prompt after a stream break than adopt the Clearing the stale state on entry closes the deferred replay without changing the fresh-prompt contract, at the cost of leaving "approval sent after a break" as a silent no-op (which it already is on this branch): // agent.ts:1637, right after the "Agent is in an interrupted state" check
if (!this._interruptState.activated) {
this._interruptState.deactivate()
}What I measuredRepro: tool interrupt → consumer
Also confirmed: the full TS unit run on this head has 2 failures in Either fix is a one-liner; which one is a design call (Python parity vs. tolerating the half-state), so flagging rather than requesting. |
Documentation Preview ReadyYour documentation preview has been successfully deployed! Changed pages: Updated at: 2026-10-28T18:05:17.680Z |
|
Assessment: Approve (re-review of The blocking issue is fixed.
What I verified
Non-blocking follow-ups from my earlier review still stand as noted in the PR description (Python Thanks for the thorough fix and the matching test rewrite — the resume behavior now matches Python. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approve. Both previous findings are fixed at 18bfe3e and the fix doesn't trade one bug for another.
- 🔴 resume silently dropped the approved tool call after a stream break → fixed:
activate()now runs before theInterruptEventyields (agent.ts:1970), mirroring the middleware path atagent.ts:1391and Python. Re-measured: tool-raised interrupt → break →'yes'runs the tool exactly once;BeforeToolCallEventgate →'no'denies it; state isactivated=false, pending=falseafterwards in both cases. - 🟡 rewritten test pinned the symptom → fixed: it now asserts the
TypeError, matching Python and the restored-state test below it. - Appendix
@throwswording → fixed.
One stale line to update, nothing else: the PR description's Behaviour change paragraph still says a new prompt "now calls the model" — it now throws TypeError: Agent is in an interrupted state… (which is the right behaviour).
What I verified / attacked
- Delta since
72fa875is onlyagent.ts(+3/−2) andagent.interrupt.test.ts; Python and docs unchanged, so I reviewed this inline rather than re-fanning out. _createInterruptResulthas exactly one caller (agent.ts:1977), so droppingactivate()from it leaves no path un-activated._streamWithMiddleware's deactivate check (agent.ts:1378-1384) is gated onstopReason !== 'interrupt', so the earlier activation doesn't get undone.- Attacks on the moved
activate()(all pass,repro-5628-followup.test.ts): snapshot taken inside theInterruptEventhandler now serialisesactivated=truesofromJSONkeeps the pending execution and a fresh agent resumes it with zero model calls; break →agent.cancel()→ resume recovers cleanly (4 messages, no replay storm); break → resume → later unrelatedAgentStreamStagegate does not replay the old tool (the stale-pending finding from last round is gone). - Full
npx vitest run --project unit-node: 5646 passed, 2 failed — bothsrc/telemetry/__tests__/config.test.tsresource-attribute tests, which fail identically on basedf200532because my sandbox exportsOTEL_RESOURCE_ATTRIBUTES. Unrelated to this PR. - Not re-run: Python suite (unchanged since last review, where the mutation test showed the parity test is load-bearing).
Reapplies two fixes from main that were dropped when the earlier rebase of 31800e8 resolved via --theirs: - fa313cc (strands-agents#5429) extract structuredOutput before the deferred append, so a message-added hook that offloads the JSON block can't lose it. - a4865cd (strands-agents#5628) activate interrupt state before yielding InterruptEvents in the catch path, so a consumer that stops at an InterruptEvent can still resume.
Reapplies two fixes from main that were dropped when the earlier rebase of 31800e8 resolved via --theirs: - fa313cc (strands-agents#5429) extract structuredOutput before the deferred append, so a message-added hook that offloads the JSON block can't lose it. - a4865cd (strands-agents#5628) activate interrupt state before yielding InterruptEvents in the catch path, so a consumer that stops at an InterruptEvent can still resume.
Reapplies two fixes from main that were dropped when the earlier rebase of 31800e8 resolved via --theirs: - fa313cc (strands-agents#5429) extract structuredOutput before the deferred append, so a message-added hook that offloads the JSON block can't lose it. - a4865cd (strands-agents#5628) activate interrupt state before yielding InterruptEvents in the catch path, so a consumer that stops at an InterruptEvent can still resume.
Human Overview
Description
Restoring a snapshot could resume a stored tool call even when the interrupt state wasn't activated.
InterruptState.fromJSONcopiedpendingToolExecutionverbatim andgetPendingExecution()never checkedactivated, while the agent's "resume only with interrupt responses" guard applies only to activated states. The next ordinary prompt therefore ran the stored tool call without a model call or an approval. Python already requiresactivatedbefore replaying (event_loop.py); this brings TypeScript in line.The gate lives in
getPendingExecution()so every caller gets it. As defense in depth,fromJSONrestores a pending execution only for an activated state and validates it (an assistant message with tool uses, completed results keyed by its tool-use ids), raisingSessionErrornaming the field instead of failing later in the loop.Behaviour change: after a consumer breaks the stream at
InterruptEvent(so the state never activates), a new prompt now calls the model instead of replaying the stored tool call.Related Issues
None
Documentation PR
Included
Type of Change
Bug fix
Testing
hatch run prepare(not run: the hatch lint env can't buildpyaudiolocally; ranhatch testand ruff directly)Additional Details
Extended context (optional)
Agent.loadSnapshotcan now throwSessionErrorfor a malformed pending tool execution in an activated state. Snapshots written by the SDK during a real interrupt always pass.resumeResponsesandcontextare left unchanged: on an activated restore they're overwritten or gated behind fresh interrupt responses, and on a non-activated restore their readers don't run. Other restored interrupt fields are out of scope here and tracked separately.from_dictwith the TypeScript restore checks is a separate follow-up.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.