feat(hitl): guard sibling double-execution when ask_user_question interrupts mid-batch#294
Conversation
…-execution A tool whose body raises a LangGraph `interrupt()` mid-execution — the `ask_user_question` shape, which suspends the run to collect a human answer — shares its in-process `Promise.all` with sibling direct tools. When it interrupts, LangGraph's resume contract re-runs the whole interrupted node from the top, so a non-idempotent sibling (send_email, billing) that already completed on the first pass runs a SECOND time on resume, duplicating the side effect. A runnable dist probe over the batch shapes (pause→resume, MemorySaver) shows the exposure is narrow: only a sibling that shares the interrupter's in-process `Promise.all` double-executes. An event-dispatched sibling is already safe — the ToolNode awaits the whole direct group (where a body interrupt unwinds) before it dispatches event tools, so a dispatched sibling never runs on the first pass. This is why the fix is scheduling within the direct group rather than idempotent resume (which would have to persist per-call completion into checkpoint state — the exact machinery the tool-error-resume work took several rounds to stabilize, unjustified for this narrow, opt-in case). Fix: add an opt-in `RunConfig.interruptingToolNames` (threaded through Graph → ToolNode, mirroring `codeSessionToolNames`). Tools named there are (1) always executed in-process — a body `interrupt()` only fires for a tool that runs inside the graph node, never for one dispatched to the host — and (2) within a batch, scheduled as their own awaited group BEFORE their non-interrupting direct siblings. If one interrupts, the batch unwinds before any sibling runs, so the sibling executes exactly once (on resume). Opt-out is byte-for-byte the prior behavior: when the set is empty or no batch call matches, direct-batch execution is an unchanged single `Promise.all`. Empirically validated (dist probe): without the set the exposed direct shapes still double-execute (2x); with it they run exactly once (1x), order-independent, in both event-driven and legacy modes; the event-dispatched sibling stays safe. New spec `src/specs/ask-user-question-batch.test.ts` pins [ask_user_question, side_effect] through pause→resume asserting the sibling runs exactly once, plus the unguarded baseline and the event-sibling-safe shape. Guards the double-execution Codex flagged on LibreChat PR #14139 (danny-avila/LibreChat#14139); the host opts in by passing the ask_user_question tool name in `interruptingToolNames`.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4b4173e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // it only ever schedules an interrupting tool ahead of its | ||
| // siblings, so propagating it into subagents can prevent a | ||
| // double side effect but never introduce one. | ||
| childGraph.interruptingToolNames = this.interruptingToolNames; |
There was a problem hiding this comment.
When a subagent keeps event-driven toolDefinitions but does not have the matching executable graphTools instance (for example self-spawned children, where buildChildInputs deliberately scrubs inherited graphTools), propagating the parent interruptingToolNames makes the child ToolNode classify that name as direct. The direct branch then invokes the schema-only stub instead of dispatching ON_TOOL_EXECUTE, so a child call to an inherited ask_user_question event definition fails with the schema-only “should not be invoked directly” error rather than reaching the host. Filter this set for child graphs to names that are actually present in the child’s graphTools, or avoid propagating it when the child cannot run the tool in-process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9241532. Root cause was folding interruptingToolNames into direct-tool classification — that promoted a schema-only event stub onto the direct path. Removed the fold-in at both sites; the set now only reorders tools that are already independently direct (real graphTools, the only kind whose body can reach interrupt()). A name that's merely an inherited event toolDefinition (e.g. a self-spawned child that scrubs graphTools) stays dispatched via ON_TOOL_EXECUTE. In LibreChat ask_user_question is a graphTool → already in directToolNames, so the guard still applies. Added a spec case asserting an interrupting name that's only an event stub is host-dispatched, never invoked directly.
…dex #294) Codex flagged that folding `interruptingToolNames` into direct-tool classification breaks a self-spawned child that scrubs inherited `graphTools` (SubagentExecutor.buildChildInputs, `config.self === true`) but keeps the event `toolDefinition`. There the name resolves only to a schema-only stub; forcing it direct invokes the stub, which throws "should not be invoked directly in event-driven mode" instead of dispatching ON_TOOL_EXECUTE to the host. Root cause: the fold-in was wrong in principle. `interruptingToolNames` should only REORDER tools that are already independently direct — a real in-process graphTool is the only kind whose body can reach `interrupt()`. A name that is merely an event `toolDefinition` has no in-process implementation to run, so promoting it to the direct path can only ever hit the stub. In the real host (LibreChat) `ask_user_question` is a graphTool and is therefore already in `directToolNames`, so the guard works without the fold-in — `runDirectBatchInterruptSafe` still schedules it ahead of its direct siblings. Fix: remove the fold-in at both classification sites (mixed-batch partition and the Send-input `isLocalTool` check). The child-graph propagation is now safe (it only reorders the child's direct group, a no-op when the child has no executable instance for the name), so it stays. Docs on `RunConfig`/`ToolNodeOptions.interruptingToolNames`, `HumanInTheLoopConfig`, and the ToolNode field are corrected to say the set reorders rather than promotes, and the tool must independently be direct for the guard to apply. New spec case pins it: a name in `interruptingToolNames` that is only a schema-only event stub is dispatched to the host (HOST-HANDLED), never invoked directly.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Problem
A tool whose body raises a LangGraph
interrupt()mid-execution — theask_user_questionshape, which suspends the run to collect a human answer — shares its in-processPromise.allwith its sibling direct tools. When it interrupts, LangGraph's resume contract re-runs the whole interrupted node from the top, so a non-idempotent sibling (send_email, billing) that already completed on the first pass runs a second time on resume, duplicating the side effect.This guards the double-execution Codex flagged on LibreChat #14139 (
danny-avila/LibreChat#14139, thread onpackages/api/src/agents/hitl/askUserQuestionTool.ts). Unlike the tool-approval HITL path (which gates the batch inPreToolUsebefore any tool runs),ask_user_questioninterrupts mid-batch, so a sibling's side effect can already have landed before the pause.Empirical scoping (dist probe, not source-reading)
A runnable probe drove
[ask_user_question, side_effect]through pause→resume against a real graph +MemorySaver, across batch shapes:The exposure is narrow: only a sibling sharing the interrupter's in-process
Promise.alldouble-executes. An event-dispatched sibling is already safe — the ToolNode awaits the whole direct group (where a body interrupt unwinds) before it dispatches event tools, so a dispatched sibling never runs on the first pass. Confirmed empirically: a direct tool body'sinterrupt()does suspend (no ALS-frame issue), and the double-execution is order-independent.Fix — schedule interrupting tools first (not idempotent resume)
Add an opt-in
RunConfig.interruptingToolNames(threadedRun → Graph → ToolNode, mirroringcodeSessionToolNames). Tools named there are:interrupt()only fires for a tool that runs inside the graph node, never for one dispatched to the host; andOpt-out is byte-for-byte the prior behavior: when the set is empty or no batch call matches, direct-batch execution is an unchanged single
Promise.all.Idempotent resume (the other candidate) was rejected: the completed sibling
ToolMessages aren't in graph state at interrupt time (the node hasn't returned), so it would require persisting per-call completion into checkpoint/store state — the exact machinery the tool-error-resume work took several rounds to stabilize — for a defect this narrow and prompt-mitigated. The scheduling guard stays entirely in execution order.Validation
src/specs/ask-user-question-batch.test.ts— pins[ask_user_question, side_effect]through pause→resume asserting the sibling runs exactly once (both orderings), plus the unguarded baseline (double) and the event-sibling-safe shape. Runs in-memory (StateGraph+MemorySaver), no LLM/Run machinery.node -eagainstdist/cjs/main.cjs, per the tool-error-resume convention): without the set the exposed direct shapes double-execute (2×); with it they run exactly once (1×), order-independent, in both modes; the event-dispatched sibling stays safe. ✅ all cases pass.directToolHITLResumeScope,directToolHooks,ToolNode.outputReferences,ToolNode.onResultCompletion,ToolNode.eagerEventExecution,toolOutputReferences) — 112 passed, 0 regressions. Typecheck + ESLint clean.Host wiring
The host opts in by passing the
ask_user_questiontool name inRunConfig.interruptingToolNames. TheHumanInTheLoopConfigJSDoc (which previously documented sibling re-execution as an unavoidable "must be idempotent" contract) is updated to describe the guard and its precise scope.