Repository navigation
🔖 fix: Preserve Ephemeral Agent Params and Identity for ask_user_question Resume - #14254
Conversation
|
@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: a32375676a
ℹ️ 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".
| if (typeof req.body.thinking !== 'boolean') { | ||
| delete req.body.thinking; |
There was a problem hiding this comment.
When the paused run used an Anthropic model where the resolved config stores thinking as { type: 'disabled' } (for example Sonnet 5 with thinking turned off), this branch deletes that object before buildEndpointOption reruns. The Anthropic config path documents that omitted thinking defaults back to adaptive/on for these models and only preserves the off state by seeing the disabled object (packages/api/src/endpoints/anthropic/helpers.ts:103-110, packages/api/src/endpoints/anthropic/llm.ts:149-157), so the resumed segment can continue with thinking enabled instead of the paused settings. Convert disabled objects to false (and preserve display/other user choices) rather than dropping every non-boolean object.
Useful? React with 👍 / 👎.
| // the rebuilt id matches the paused one. No-op when the spec has no preset.modelLabel | ||
| // (both turns then already agree on modelSpec.label). | ||
| const spec = req.body?.spec; | ||
| if (spec && req.body.modelLabel == null) { |
There was a problem hiding this comment.
When the resume payload still includes modelLabel—including an empty string from client defaults or a stale label—this guard skips the server-side replay and leaves that value authoritative. For custom-endpoint modelSpec ephemeral agents, loadEphemeralAgent uses model_parameters.modelLabel ?? modelSpec.label to form the ephemeral id/checkpoint namespace, while the resume fingerprint does not include modelLabel; a stale/empty label therefore rebuilds a different id and the ask_user_question/tool checkpoint still cannot be re-entered. The replay should overwrite or clear client-supplied modelLabel, not only fill null/undefined.
Useful? React with 👍 / 👎.
|
Thanks @jjhidalgar-celonis, great diagnosis on all three bugs in #14253. Heads up: I'm about to push to this branch (maintainer edits) to reconcile it with what already landed:
More shortly. |
# Conflicts: # packages/api/src/stream/implementations/RedisJobStore.ts
|
Pushed (9fd0a4d) and retargeted to
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fd0a4d58c
ℹ️ 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".
| type?: unknown; | ||
| budget_tokens?: unknown; | ||
| }; | ||
| params.thinking = type !== 'disabled'; |
There was a problem hiding this comment.
When resuming Anthropic adaptive-thinking runs where the resolved config is thinking: { type: 'adaptive', display: 'omitted' }, this collapses the object to thinking: true without copying display to thinkingDisplay. The Anthropic initializer explicitly relies on either options.modelOptions.thinkingDisplay or the persisted object’s .display to avoid demoting an explicit user choice back to the default (packages/api/src/endpoints/anthropic/llm.ts lines 133-170), so after this replay path the resumed run can switch Opus 4.7+/Sonnet 5+ from omitted thoughts to the default summarized display.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in aa1301e. normalizeThinkingParam now copies the adaptive object's display to top-level thinkingDisplay (when not already set), so an explicit omitted choice survives the resume round-trip instead of demoting to the Opus 4.7+ default. Covered by a new test in policy.spec.ts.
| } | ||
| return sanitizeParamValue(params, 0) as Record<string, unknown>; | ||
| const sanitized = sanitizeParamValue(params, 0) as Record<string, unknown>; | ||
| normalizeThinkingParam(sanitized); |
There was a problem hiding this comment.
For native Anthropic adaptive-thinking runs with a non-default effort, the resolved llmConfig stores that choice as invocationKwargs.output_config.effort (configureReasoning writes it there), while the resume parser only accepts a top-level effort field. This sanitizer now makes those runs parseable by converting the thinking object, but it returns invocationKwargs unchanged; restoreResumeContext assigns it to req.body and parseCompactConvo drops it, so the resumed turn silently falls back to the default effort instead of the paused run’s setting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in aa1301e. New normalizeEffortParam lifts invocationKwargs.output_config.effort back to the top-level effort field the request-body schema accepts (existing top-level value wins), and drops invocationKwargs from the replayed params since the compact-convo schema would discard it anyway. Covered by a new test in policy.spec.ts.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
@codex review |
danny-avila
changed the title
danny-avila
changed the title
ask_user_question Resume
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
Pushed 0e0c4de, closing out a broader gap in the same mechanism: replaying only the RESOLVED llmConfig loses any param the provider resolution renames, relocates, or retypes. The Fix: @codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
danny-avila
changed the title
ask_user_question Resume
…tion Resume (LibreChat-AI#14254) * fix: durable ask_user_question resume for ephemeral agents * 🤖 refactor: Drop chat.js resume hunks in favor of shared packages/api helpers * 🤖 fix: Normalize resume thinking param and replay modelLabel (LibreChat-AI#14253 Bugs 1&2) * 🤖 fix: Preserve adaptive thinking display and effort across HITL resume * 🔤 style: Sort load.spec.ts imports (repo import-order) * 🤖 fix: Replay paused request body params on HITL resume (UI-form source of truth) --------- Co-authored-by: Danny Avila <danny@librechat.ai>
…tion Resume (LibreChat-AI#14254) * fix: durable ask_user_question resume for ephemeral agents * 🤖 refactor: Drop chat.js resume hunks in favor of shared packages/api helpers * 🤖 fix: Normalize resume thinking param and replay modelLabel (LibreChat-AI#14253 Bugs 1&2) * 🤖 fix: Preserve adaptive thinking display and effort across HITL resume * 🔤 style: Sort load.spec.ts imports (repo import-order) * 🤖 fix: Replay paused request body params on HITL resume (UI-form source of truth) --------- Co-authored-by: Danny Avila <danny@librechat.ai>
2 participants
Summary
ask_user_question(#14139) fails to resume on ephemeral agents (reported on acustom endpoint with a
modelSpec, but the mechanism applies to any providerendpoint; persisted agents are unaffected since their id and params are DB-sourced).
Three independent bugs were diagnosed in #14253; Bug 3 (the
RedisJobStoredisposed-graph crash) landed separately in #14258 with a Redis regression test, so
this PR fixes the remaining two, plus the wider parameter-fidelity gap behind Bug 1.
The fixes live in the shared resume helpers in
packages/api(wheresanitizeResumeModelParameters/RESUME_CONTEXT_KEYSalready own this logic), soboth persisted paths stay covered by unit tests:
missing_modelon answer — the resolved Anthropicthinkingparam is anobject (
{ type, budget_tokens }), butparseCompactConvoonly accepts aboolean. Replaying it onto the resume body fails the compact-convo schema, which
silently returns
{}and dropsmodel/spec, so the ephemeral agent rebuildswith no model. Fix:
sanitizeResumeModelParametersnormalizes the object back tothe request-body form (
thinking: boolean+thinkingBudget+thinkingDisplay)and lifts adaptive
effortout ofinvocationKwargs.output_config.Silent no-op resume — the ephemeral agent id (LangGraph node name → HITL
checkpoint namespace) embeds
sender = model_parameters.modelLabel ?? modelSpec.label.modelLabelis stripped from the resolved llmConfig capturedfor resume, so resume rebuilds a different id, can't re-enter the interrupt
checkpoint, and continues with an empty graph. Fix:
modelLabeljoinsRESUME_CONTEXT_KEYS, so the original request's value is captured at pause andreplayed on resume (the delete-absent half of
applyResumeContextblocks acrafted resume from injecting one).
Silent param resets on resume (same class as Bug 1) — replaying only the
resolved llmConfig loses any param the provider resolution renames, relocates, or
retypes: Anthropic
maxOutputTokens→maxTokensandstop→stopSequences,the snake_case family on the OpenAI path (
top_p,presence_penalty,max_tokens,reasoning_effort→reasoning), andmaxContextTokens(never inthe llmConfig at all) — all silently reset to defaults on resume. Fix:
captureResumeModelParameterscaptures the paused request body's generationparams as the primary source (UI-form by construction, so replay can't trip the
schema); the resolved llmConfig only fills gaps, still sanitized/normalized. The
replay key set derives from the compact-convo schemas' own shapes — exactly the
fields that can influence the rebuilt run.
reading 'slice'/contentData is not iterable(
readCachedGraphguard + Redis regression test), now merged intodev.Fixes #14253
Related: #14139, #14258
Test plan
packages/api—policy.spec.tscovers thethinkingnormalization (object → boolean + budget + display), the
effortlift, themodelLabelcapture/replay/anti-injection, andcaptureResumeModelParameters(body-first precedence, gap-fill, identity-field exclusion, sanitization);
load.spec.tsdocuments the Bug 2 mechanism (ephemeral id drifts whenmodelLabelis lost, stable when kept).modelSpecwithaskUserQuestion: trueand apreset.modelLabel, using Claude Opus/Sonnet 4+. Triggerask_user_question,tap an option:
missing_modelerror,paused turn.