[pull] main from danny-avila:main#132
Merged
Merged
Conversation
) * fix: tool error completion contract across interrupt/resume passes A direct (graphTools) tool that fails fast — e.g. a zod schema reject — on the RESUME pass of an interrupted batch errors before the rebuilt graph has registered run steps for the replayed calls. handleToolCallErrorStatic threw ('No config provided') in that state, surfacing a scary 'Error in errorHandler' log on every such resume even though the error itself was already handled (the model receives the error ToolMessage regardless). - handleToolCallErrorStatic now returns whether it dispatched the error completion instead of throwing on missing config/stepId/runStep; the unused config precondition is dropped (the dispatch never consumed it). - ToolNode records calls whose errorHandler did NOT dispatch (returned false or threw) and lets its own completion loop dispatch for exactly those — the skip-when-errorHandler-exists shortcut previously assumed the handler always succeeded, stranding the client's tool-call part without a terminal event whenever it didn't. - Net contract: the error completion dispatches exactly once, on the pass where the call's run step is live; resume re-executions of an already-reported error stay silent toward the client (no duplicate cards), and the model keeps receiving the error ToolMessage on every pass. Repro (LibreChat #14139 field report): ask_user_question sibling batch where one call exceeded the 12-option schema cap; verified with a dist probe (pause -> resume on a fresh instance) and covered by src/specs/tool-error-resume.test.ts. * test: fix strict-tool assertion to match the schema error wrapper The strict tool used z.number().max(1), whose zod message is 'Number must be less than or equal to 1' — the 'at most 1' assertion never matched (the suite can't load locally due to the pre-existing mistralai ESM issue, so this was missed pre-push). Switch to an array-cap schema (z.array().max(1)), which faithfully mirrors the field repro — an ask_user_question sibling whose options array exceeded the 12-cap — and assert on the version-independent LangChain wrapper 'did not match expected schema'. Verified locally with the mistralai import stubbed. * fix: consume undispatched-error markers instead of accumulating them Codex #292 review: ids added to `undispatchedToolErrors` (when errorHandler reports it could not dispatch) were never removed — an add-only set. Now the completion loop DELETEs the id when it takes over the dispatch (consume), and `run()` clears the set at batch entry so a marker left behind by an invocation that interrupted before its completion loop can't survive into a resume re-execution. Probed the specific double-dispatch scenario (same-instance resume re-entering the same tool_call_id): NOT reproducible in the current SDK — the marker is only added on a fresh rebuilt instance whose step replay hasn't registered the id yet, and that instance never re-enters with the same id succeeding. So this is defensive hardening against a future dispatch-ordering change plus removing the add-only-set growth, not a live-bug fix. Existing tool-error-resume spec (exactly-once across resume) still green; verified no regression via rebuilt- and same-instance dist probes. * fix: drop the racing run()-entry clear of undispatched-error markers Codex #292 re-review: clearing the shared `undispatchedToolErrors` set at the start of every run() races with a concurrent/overlapping invocation on the same ToolNode — a second batch's clear wipes the first batch's markers before its completion loop runs, so the first error ToolMessage hits the errorHandler-skip path and never dispatches a terminal completion (the very missing-completion case this PR fixes). Remove the clear. The consume-and-delete in the completion loop is the sole mechanism now, and it is concurrency-safe on its own: markers are keyed by globally-unique tool_call_id, so concurrent batches touch disjoint entries and never each other's. Each marker is reclaimed when its completion dispatches; the only residual is a marker left by an invocation that interrupted before its loop ran, which rides a short-lived rebuilt instance and is GC'd with it. Both dist probes (rebuilt- and same-instance resume) stay exactly-once. * fix: tighten the error-completion dispatch contract (Codex #292) Two P2s on the boolean contract: 1. Graph.ts handleToolCallErrorStatic returned true even when NO ON_RUN_STEP_COMPLETED handler was registered (the optional-chained dispatch was a no-op). Hosts that wire completions through callback-based custom events instead of a registered handler would have the error completion silently dropped. Now look up the handler first and return false when none exists, so the ToolNode runs its fallback dispatch. 2. ToolNode runTool marked a THROWN errorHandler as undispatched. But a throw is not proof nothing dispatched: the built-in session handler emits tool.completed BEFORE invoking a user ON_RUN_STEP_COMPLETED callback, so a throw there has already dispatched — re-marking it made the fallback loop emit a DUPLICATE completion (double stream event / UI card). Only an explicit false return now means 'nothing dispatched'; a throw is just logged. Both dist probes stay exactly-once; tsc clean.
…errupts mid-batch (#294) * feat(hitl): schedule interrupting tools first to guard sibling double-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`. * fix(hitl): don't force interruptingToolNames onto the direct path (Codex #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.
…dependencies to latest versions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )