feat: continue runs from blocking stop hooks - #482
Conversation
4a9204e to
8a81123
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8a81123610
ℹ️ 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".
| messages: this.hasCheckpointer | ||
| ? injected | ||
| : [...graph.messages, ...injected], |
There was a problem hiding this comment.
When a run was started or resumed with configurable.checkpoint_id, this continuation submits only the injected delta but reuses the original pinned checkpoint configuration on the next streamEvents call. LangGraph therefore applies the steer to the historical/interrupted checkpoint rather than the state completed by the preceding segment, losing that segment's answer and tool state or replaying the interrupt. Remove or replace the pinned checkpoint ID after the first segment, or seed the continuation from the complete current state.
Useful? React with 👍 / 👎.
| const stream = graphRunnable.streamEvents( | ||
| streamInputs as t.IState, | ||
| { ...config, runName: graph.runName }, |
There was a problem hiding this comment.
When a Stop hook admits a continuation, each loop iteration calls streamEvents again with the same top-level callbacks, producing another detached AgentGraph root rather than one root for the processStream lifecycle. In Langfuse this fragments the conversation across sibling roots, and with a checkpointer the later root's input is only the injected delta rather than the user's conversation. Parent continuation segments beneath the original root or otherwise aggregate them into one correctly shaped root observation.
AGENTS.md reference: AGENTS.md:L143-L145
Useful? React with 👍 / 👎.
| const stream = graphRunnable.streamEvents( | ||
| streamInputs as t.IState, | ||
| { ...config, runName: graph.runName }, |
There was a problem hiding this comment.
When no checkpointer is configured, the second streamEvents invocation resets LangGraph's langgraph_step numbering while the retained graph sidecars still contain the first segment's stepKeyIds and messageIdsByStepKey. Because getStepKey is derived from the run, thread, node, and step number, a direct-answer continuation reuses the first segment's key; getMessageId then declines to create a new message and subsequent deltas are sent to the prior closed step. Clients consequently receive no new message step and can append the continued answer to the pre-steer answer, so add a segment identity to event keys or rotate the per-segment mappings.
Useful? React with 👍 / 👎.
| }).catch((): undefined => undefined); | ||
| stopResult = mergeAggregatedHookResults(stopResult, finalized); |
There was a problem hiding this comment.
When a durable StopFinalize callback throws or times out after atomically claiming queued input, executeHooks records the failure in errors rather than throwing, while call-level failures are also converted to undefined; this merge ignores both cases and lets processStream terminate normally. The claimed message is then neither injected nor available for a later run, causing durable input loss. Treat a finalizer error as an indeterminate terminal-admission failure that propagates or otherwise requires host reconciliation instead of silently sealing the run.
Useful? React with 👍 / 👎.
| let streamInputs: t.IState | Command = inputs; | ||
| let stopContinuationCount = 0; |
There was a problem hiding this comment.
When a continued segment pauses for a human-in-the-loop interrupt, the next Run.resume() enters a new processStream call and resets this counter to zero even though it is resuming the same run execution. The next Stop/StopFinalize inputs therefore report stopHookActive: false, an incorrect count, and a replenished budget; repeated continuation-to-interrupt cycles can bypass maxStopContinuations indefinitely. Store the admitted count in run/checkpoint state that survives a clean interrupt and reset it only for a genuinely fresh execution.
Useful? React with 👍 / 👎.
|
@codex review Please review the current PR head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54832a1863
ℹ️ 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".
| await this.resolveInterruptResumeConfig(config); | ||
| } | ||
| if (this._interrupt != null) { | ||
| await this.resolveInterruptResumeConfig(config); |
There was a problem hiding this comment.
When a run starts from a pinned checkpoint_id, admits a terminal continuation, and that continued segment pauses for HITL, this passes the original pinned config to interrupt resolution rather than the advanced streamConfig. A rebuilt Run.resume() that retains the caller's pin can consequently inspect and resume the pre-continuation checkpoint, losing the warm segment and its persisted continuation budget. Fresh evidence at this head is that the cursor is advanced only for the next streamEvents call, while this interrupt-resolution path still uses the old configuration.
Useful? React with 👍 / 👎.
| if (runId === this.deferredRootRunId && parentRunId == null) { | ||
| this.deferredRootOutcome = { type: 'end', output }; | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
When StopFinalize throws or times out after a graph segment ends, this records the segment's successful chain outcome, and disposeLangfuseHandler() later replays that success even though processStream() rejects and fires StopFailure. Because post-stream hook errors never invoke this handler's handleChainError, Langfuse reports a successful agent root for an indeterminate terminal-admission failure; the deferred root outcome needs to reflect the enclosing process lifecycle, not only the last graph callback.
AGENTS.md reference: AGENTS.md:L122-L124
Useful? React with 👍 / 👎.
| this.stopContinuationCount = state.stopContinuationCount ?? 0; | ||
| this.streamSegment = state.streamSegment ?? 0; |
There was a problem hiding this comment.
When a checkpointer is reused for the next ordinary user turn, its latest runStepState still contains the prior turn's terminal-continuation count. Although resetValues() clears the in-memory counter for the fresh processStream() call, the first graph node restores that persisted value here, so stopHookActive is immediately true and the remaining budget shrinks cumulatively across unrelated turns; after enough turns, valid continuations are rejected as exhausted. Restore these lifecycle fields only for HITL resume, or overwrite them in graph state when starting a genuinely fresh execution.
Useful? React with 👍 / 👎.
| delete configurable.checkpoint_id; | ||
| delete configurable.checkpoint_map; |
There was a problem hiding this comment.
When two executions can write checkpoints for the same thread, removing the pinned checkpoint cursor makes the warm segment load whichever checkpoint is globally latest, not necessarily the checkpoint produced by its preceding segment. A concurrent turn or sibling fork writing between the two streamEvents calls can therefore splice that other branch's messages and tool state into this continuation. Fresh evidence in this head is that advanceCheckpointCursor() deletes the branch identifiers rather than replacing them with the completed segment's exact checkpoint configuration.
Useful? React with 👍 / 👎.
|
@codex review Please review the current PR head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63cc950fef
ℹ️ 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".
| runStepState?.stopContinuationExecutionId !== executionId || | ||
| runStepState.streamSegment !== streamSegment |
There was a problem hiding this comment.
When a HITL checkpoint created before this commit is resumed after an upgrade, its optional stopContinuationExecutionId is absent, while the restored graph uses ''; the resumed segment consequently persists the field as absent again. If a Stop hook then admits a continuation, this strict comparison rejects every checkpoint (undefined !== '') and throws Cannot identify the checkpoint committed by the completed graph segment. Normalize the legacy missing value or stamp a new execution ID during resume so in-flight HITL runs survive rolling upgrades.
Useful? React with 👍 / 👎.
| sessionId: this.id, | ||
| signal: config.signal, | ||
| }); | ||
| assertFinalAdmissionSucceeded(finalized); |
There was a problem hiding this comment.
When a StopFinalize matcher is registered with internal: true and its hook throws or times out after atomically claiming input, executeHooks deliberately omits that failure from AggregatedHookResult.errors, so this assertion treats admission as successful and the run terminates normally while the claimed message is lost. Fresh evidence at this head is that terminal admission now relies exclusively on the filtered errors array; preserve finalizer success/failure independently of the matcher's diagnostic-suppression flag.
Useful? React with 👍 / 👎.
|
@codex review Please review the current PR head |
|
Codex Review: Didn't find any major issues. Swish! 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
Summary
StopDecision: blockcontract by admitting injected messages into another graph segment inside the sameRun.processStreamlifecycleStopFinalizeadmission phase after parallelStophooks are folded, so durable hosts can atomically claim or seal with full knowledge of other continuation sourcesRunStartorUserPromptSubmitWhy
LibreChat needs an atomic terminal steer handoff: a steer accepted immediately before natural completion should either continue the already-warm run or remain an ordinary follow-up after terminal admission is sealed. The SDK must own the in-run continuation lifecycle and serialize final admission after notification/plugin Stop hooks before the host can implement that storage transition safely.
Validation
tsc --noEmit