From e813934731651323bcdfaa2205977105ac90a02c Mon Sep 17 00:00:00 2001 From: Jaime Hidalgo Date: Tue, 14 Jul 2026 21:12:43 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=96=20fix:=20Preserve=20Ephemeral=20Ag?= =?UTF-8?q?ent=20Params=20and=20Identity=20for=20ask=5Fuser=5Fquestion=20R?= =?UTF-8?q?esume=20(#14254)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 (#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 --- api/server/controllers/agents/client.js | 22 +-- packages/api/src/agents/hitl/policy.spec.ts | 161 ++++++++++++++++++++ packages/api/src/agents/hitl/policy.ts | 134 +++++++++++++++- packages/api/src/agents/load.spec.ts | 52 +++++++ 4 files changed, 357 insertions(+), 12 deletions(-) create mode 100644 packages/api/src/agents/load.spec.ts diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index eb1a11fa26..a2ad4be54c 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -37,7 +37,7 @@ const { toClientPendingAction, computeAgentRequestFingerprint, extractDiscoveredToolsFromHistory, - sanitizeResumeModelParameters, + captureResumeModelParameters, pickResumeContext, getApprovalTtlMs, isHITLEnabled, @@ -1378,19 +1378,21 @@ class AgentClient extends BaseClient { const appConfig = this.options.req?.config; const checkpointerCfg = appConfig?.endpoints?.[EModelEndpoint.agents]?.checkpointer; - // Persist the resolved model parameters (temperature, max tokens, custom endpoint - // params, …) so an ephemeral-agent resume continues with the SAME settings the run - // paused on. The resume payload omits them and they aren't part of the fingerprint, so - // without this the rebuilt ephemeral run falls back to defaults. (Saved agents source - // these from the DB record server-side, so this is belt-and-suspenders for them.) - // Sanitized: the resolved params are the llmConfig, which carries provider secrets + // Persist the generation params (temperature, max tokens, custom endpoint params, …) + // so an ephemeral-agent resume continues with the SAME settings the run paused on. + // The resume payload omits them and they aren't part of the fingerprint, so without + // this the rebuilt ephemeral run falls back to defaults. The paused request body is + // the primary source (UI-form, round-trips the compact-convo schema by construction); + // the resolved llmConfig fills gaps and is sanitized — it carries provider secrets // (apiKey, credentials) and gateway config — resume re-resolves those server-side. + // (Saved agents source params from the DB record, so this is belt-and-suspenders.) const resumeContext = pickResumeContext(this.options.req?.body); - const resolvedModelParameters = sanitizeResumeModelParameters( + const resumeModelParameters = captureResumeModelParameters( + this.options.req?.body, this.options.agent?.model_parameters, ); - if (resolvedModelParameters) { - resumeContext.model_parameters = resolvedModelParameters; + if (resumeModelParameters) { + resumeContext.model_parameters = resumeModelParameters; } // Persist the question onto the paused ask tool_call's args NOW: an // abandoned/expired/stopped pause never reaches the answer-resume stamp, diff --git a/packages/api/src/agents/hitl/policy.spec.ts b/packages/api/src/agents/hitl/policy.spec.ts index fd064418e9..46a6e3175e 100644 --- a/packages/api/src/agents/hitl/policy.spec.ts +++ b/packages/api/src/agents/hitl/policy.spec.ts @@ -8,6 +8,7 @@ import { buildPendingAction, toClientPendingAction, computeAgentRequestFingerprint, + captureResumeModelParameters, sanitizeResumeModelParameters, pickResumeContext, applyResumeContext, @@ -356,6 +357,151 @@ describe('sanitizeResumeModelParameters', () => { expect(sanitizeResumeModelParameters('sk-secret')).toBeUndefined(); expect(sanitizeResumeModelParameters(['sk-secret'])).toBeUndefined(); }); + + test('normalizes the resolved Anthropic object `thinking` back to the request-body form (#14253)', () => { + // Opus/Sonnet 4+ resolve `thinking` to a provider-format object; replaying it + // verbatim fails the compact-convo `thinking: z.boolean()` field and its + // `.catch(()=>({}))` drops model/spec → missing_model. + expect( + sanitizeResumeModelParameters({ + model: 'claude-opus-4-20250514', + thinking: { type: 'enabled', budget_tokens: 2048 }, + }), + ).toEqual({ model: 'claude-opus-4-20250514', thinking: true, thinkingBudget: 2048 }); + + expect(sanitizeResumeModelParameters({ thinking: { type: 'disabled' } })).toEqual({ + thinking: false, + }); + + // Boolean thinking (and an explicit thinkingBudget) are left untouched. + expect(sanitizeResumeModelParameters({ thinking: true, thinkingBudget: 4096 })).toEqual({ + thinking: true, + thinkingBudget: 4096, + }); + expect( + sanitizeResumeModelParameters({ + thinking: { type: 'enabled', budget_tokens: 2048 }, + thinkingBudget: 4096, + }), + ).toEqual({ thinking: true, thinkingBudget: 4096 }); + }); + + test('preserves an explicit adaptive `display` as thinkingDisplay (#14253)', () => { + // Opus 4.7+ adaptive configs carry `display`; dropping it would demote an + // explicit 'omitted' choice back to the default ('summarized') on resume. + expect( + sanitizeResumeModelParameters({ thinking: { type: 'adaptive', display: 'omitted' } }), + ).toEqual({ thinking: true, thinkingDisplay: 'omitted' }); + // An explicit top-level thinkingDisplay wins over the object's display. + expect( + sanitizeResumeModelParameters({ + thinking: { type: 'adaptive', display: 'summarized' }, + thinkingDisplay: 'omitted', + }), + ).toEqual({ thinking: true, thinkingDisplay: 'omitted' }); + }); + + test('lifts adaptive effort out of invocationKwargs.output_config (#14253)', () => { + // configureReasoning stores a non-default effort at + // invocationKwargs.output_config.effort; the request-body schema only accepts + // the top-level field, so replaying without the lift loses the effort choice. + expect( + sanitizeResumeModelParameters({ + thinking: { type: 'adaptive' }, + invocationKwargs: { metadata: { user_id: 'u1' }, output_config: { effort: 'max' } }, + }), + ).toEqual({ thinking: true, effort: 'max' }); + // An existing top-level effort wins; invocationKwargs is always dropped. + expect( + sanitizeResumeModelParameters({ + effort: 'low', + invocationKwargs: { output_config: { effort: 'max' } }, + }), + ).toEqual({ effort: 'low' }); + expect( + sanitizeResumeModelParameters({ invocationKwargs: { metadata: { user_id: 'u1' } } }), + ).toEqual({}); + }); +}); + +describe('captureResumeModelParameters', () => { + test('captures UI-form body params the resolved llmConfig renames or drops (#14253)', () => { + // Anthropic resolution renames maxOutputTokens → maxTokens and stop → stopSequences; + // replaying only the resolved form would silently reset those on resume. + expect( + captureResumeModelParameters( + { + text: 'hi', + maxOutputTokens: 8192, + stop: ['END'], + temperature: 0.3, + maxContextTokens: 50000, + }, + { model: 'claude-opus-4', temperature: 0.3, maxTokens: 8192, stopSequences: ['END'] }, + ), + ).toEqual({ + model: 'claude-opus-4', + temperature: 0.3, + maxTokens: 8192, + stopSequences: ['END'], + maxOutputTokens: 8192, + stop: ['END'], + maxContextTokens: 50000, + }); + }); + + test('body values win over the normalized resolved values', () => { + expect( + captureResumeModelParameters( + { thinking: false, effort: 'low' }, + { thinking: { type: 'adaptive' }, invocationKwargs: { output_config: { effort: 'max' } } }, + ), + ).toEqual({ thinking: false, effort: 'low' }); + }); + + test('resolved params still fill gaps the body lacks (normalized to UI form)', () => { + expect( + captureResumeModelParameters( + {}, + { + thinking: { type: 'adaptive', display: 'omitted' }, + invocationKwargs: { output_config: { effort: 'max' } }, + }, + ), + ).toEqual({ thinking: true, thinkingDisplay: 'omitted', effort: 'max' }); + expect(captureResumeModelParameters({ temperature: 0.5 }, undefined)).toEqual({ + temperature: 0.5, + }); + }); + + test('only replays schema-known generation params; identity fields stay owned elsewhere', () => { + // model/spec/modelLabel/promptPrefix ride RESUME_CONTEXT_KEYS; text/files/etc. + // never reach model_parameters (parseCompactConvo strips them). + expect( + captureResumeModelParameters( + { + model: 'gpt-5', + spec: 'my-spec', + modelLabel: 'My Opus', + promptPrefix: 'be nice', + text: 'hello', + conversationId: 'c1', + top_p: 0.9, + }, + undefined, + ), + ).toEqual({ top_p: 0.9 }); + expect(captureResumeModelParameters({ text: 'hello' }, undefined)).toBeUndefined(); + }); + + test('sanitizes sensitive keys inside captured body values', () => { + expect( + captureResumeModelParameters( + { additionalModelRequestFields: { apiKey: 'sk-live', anthropic_beta: ['x'] } }, + undefined, + ), + ).toEqual({ additionalModelRequestFields: { anthropic_beta: ['x'] } }); + }); }); describe('computeAgentRequestFingerprint', () => { @@ -434,6 +580,8 @@ describe('pickResumeContext / applyResumeContext', () => { timezone: 'America/New_York', // Graph-determining: skill allowed-tools union into the tool set. manualSkills: ['code-reviewer'], + // Graph-determining: feeds the ephemeral agent id / checkpoint namespace (#14253). + modelLabel: 'My Opus', conversationId: 'c', decisions: [], actionId: 'x', @@ -447,9 +595,22 @@ describe('pickResumeContext / applyResumeContext', () => { addedConvo: { agent_id: 'secondary' }, timezone: 'America/New_York', manualSkills: ['code-reviewer'], + modelLabel: 'My Opus', }); }); + it('replays a dropped modelLabel so the ephemeral agent id stays stable (#14253)', () => { + // Resume/reload case: the resolved llmConfig stripped modelLabel; the server restores + // the original top-level value so parseCompactConvo re-derives the same sender/id. + const restored: Record = { conversationId: 'c', actionId: 'x' }; + applyResumeContext(restored, { endpoint: 'my-custom-endpoint', modelLabel: 'My Opus' }); + expect(restored.modelLabel).toBe('My Opus'); + // A paused turn with no modelLabel can't be made to inject one. + const injected: Record = { conversationId: 'c', modelLabel: 'Spoofed' }; + applyResumeContext(injected, { endpoint: 'my-custom-endpoint' }); + expect('modelLabel' in injected).toBe(false); + }); + it('replays a dropped manualSkills and drops a client-injected one', () => { // Reload case: the resume client lost manualSkills; the server restores it. const restored: Record = { conversationId: 'c', actionId: 'x' }; diff --git a/packages/api/src/agents/hitl/policy.ts b/packages/api/src/agents/hitl/policy.ts index 113884b1be..5550873322 100644 --- a/packages/api/src/agents/hitl/policy.ts +++ b/packages/api/src/agents/hitl/policy.ts @@ -1,4 +1,5 @@ import { randomUUID, createHash } from 'crypto'; +import { openAIBaseSchema, googleBaseSchema, anthropicBaseSchema } from 'librechat-data-provider'; import type { Agents, TToolApprovalPolicy } from 'librechat-data-provider'; import type { ToolPolicyConfig } from '@librechat/agents'; @@ -236,6 +237,16 @@ export const RESUME_CONTEXT_KEYS = [ // different skill's tools (manualSkills isn't covered by the fingerprint). Replay-only. // (alwaysAppliedSkills is NOT here — it's resolved server-side from the DB, not req.body.) 'manualSkills', + // Graph-determining for ephemeral agents: `loadEphemeralAgent` encodes the agent id + // (and thus the LangGraph node name / HITL checkpoint namespace) from + // `sender = modelLabel ?? modelSpec.label ?? …`. `modelLabel` is stripped from the + // RESOLVED llmConfig captured at pause (sanitizeResumeModelParameters reads the + // initialized agent's model_parameters), so without replaying the original request + // value the resumed id falls back to modelSpec.label → a DIFFERENT id → the interrupt + // checkpoint (namespaced by the paused id) can't be re-entered → empty-graph resume + // (#14253). It rides top-level on req.body and flows into model_parameters via the + // build spread, so replaying it here restores the stable id. Replay-only. + 'modelLabel', ] as const; export type ResumeContext = Partial> & { @@ -328,6 +339,59 @@ function sanitizeParamValue(value: unknown, depth: number): unknown { return value; } +/** + * The resolved Anthropic `thinking` parameter is a provider-format object + * (`{ type: 'enabled' | 'disabled' | 'adaptive', budget_tokens? }`) for Opus/Sonnet + * 4+, but the request body — and the compact-convo schema (`thinking: z.boolean()`) + * that the resume replay is validated against — expects the UI form. A stray object + * fails that field, and the schema's `.catch(() => ({}))` drops the WHOLE parse + * (`model`/`spec` included), surfacing as `missing_model` on resume of a + * custom-endpoint ephemeral agent (#14253). Convert it back to + * `{ thinking: boolean, thinkingBudget?, thinkingDisplay? }` so the replayed params + * round-trip cleanly (an explicit `display: 'omitted'` choice survives too). + */ +function normalizeThinkingParam(params: Record): void { + const thinking = params.thinking; + if (thinking == null || typeof thinking !== 'object' || Array.isArray(thinking)) { + return; + } + const { + type, + display, + budget_tokens: budget, + } = thinking as { + type?: unknown; + display?: unknown; + budget_tokens?: unknown; + }; + params.thinking = type !== 'disabled'; + if (params.thinkingBudget == null && typeof budget === 'number') { + params.thinkingBudget = budget; + } + if (params.thinkingDisplay == null && typeof display === 'string') { + params.thinkingDisplay = display; + } +} + +/** + * A non-default adaptive-thinking effort resolves into + * `invocationKwargs.output_config.effort` (see `configureReasoning`), while the + * request-body schema only accepts a top-level `effort`. Lift it back so the resumed + * turn keeps the paused run's effort, and drop `invocationKwargs` entirely — it's + * resolved transport config the compact-convo schema would discard anyway. + */ +function normalizeEffortParam(params: Record): void { + const kwargs = params.invocationKwargs as { output_config?: { effort?: unknown } } | undefined; + if (kwargs == null || typeof kwargs !== 'object') { + return; + } + const effort = kwargs.output_config?.effort; + if (params.effort == null && typeof effort === 'string') { + params.effort = effort; + } + delete params.invocationKwargs; +} + /** * Strip credentials and server transport config from resolved model parameters before * they are persisted for resume replay. The initialized agent's `model_parameters` are @@ -335,7 +399,9 @@ function sanitizeParamValue(value: unknown, depth: number): unknown { * Google `authOptions`, Bedrock `credentials`) and gateway config (`configuration`, * headers, base URLs). Resume re-resolves all of those server-side from env/config, so * only the user-level generation params (temperature, max tokens, custom endpoint - * params, …) need to survive the round trip. + * params, …) need to survive the round trip. Provider-format params that conflict with + * the request-body schema on replay are normalized back to the UI form (see + * {@link normalizeThinkingParam}). */ export function sanitizeResumeModelParameters( params: unknown, @@ -343,7 +409,71 @@ export function sanitizeResumeModelParameters( if (params == null || typeof params !== 'object' || Array.isArray(params)) { return undefined; } - return sanitizeParamValue(params, 0) as Record; + const sanitized = sanitizeParamValue(params, 0) as Record; + normalizeThinkingParam(sanitized); + normalizeEffortParam(sanitized); + return sanitized; +} + +/** Bedrock body params its compact schema accepts; hand-listed because + * `bedrockInputSchema` wraps the pick in a transform, hiding `.shape`. */ +const BEDROCK_PARAM_KEYS = [ + 'region', + 'system', + 'maxTokens', + 'reasoning_effort', + 'additionalModelRequestFields', +]; + +/** Schema-accepted keys owned elsewhere: replayed via {@link RESUME_CONTEXT_KEYS} + * (`model`, `spec`, `promptPrefix`, `modelLabel`) or derived server-side / identity + * fields the resume request must keep as its own. */ +const RESUME_PARAM_EXCLUDED = new Set([ + 'model', + 'spec', + 'iconURL', + 'greeting', + 'modelLabel', + 'promptPrefix', + 'chatProjectId', +]); + +/** + * Request-body generation params worth replaying on resume: the union of the + * compact-convo schemas' fields. Only these keys can influence the rebuilt run — + * `buildOptions` derives `model_parameters` from the PARSED body, and + * `parseCompactConvo` strips everything else. + */ +const RESUME_PARAM_KEYS: string[] = Array.from( + new Set( + [openAIBaseSchema, anthropicBaseSchema, googleBaseSchema] + .flatMap((schema) => Object.keys(schema.shape)) + .concat(BEDROCK_PARAM_KEYS), + ), +).filter((key) => !RESUME_PARAM_EXCLUDED.has(key)); + +/** + * Capture the model parameters to replay on resume. The paused request body is the + * primary source — its fields are UI-form by construction (they already round-tripped + * `parseCompactConvo` on the original turn), so replaying them can't trip the schema. + * The resolved llmConfig only fills gaps: it's provider-format, where params are + * renamed (`maxOutputTokens` → `maxTokens`, `top_p` → `topP`), relocated + * (`effort` → `invocationKwargs`), or retyped (`thinking` → object) — the schema + * silently drops or, worse, fails on them (see the `normalize*` helpers, #14253). + */ +export function captureResumeModelParameters( + body: Record | undefined | null, + resolvedParams: unknown, +): Record | undefined { + const captured = sanitizeResumeModelParameters(resolvedParams) ?? {}; + if (body != null && typeof body === 'object') { + for (const key of RESUME_PARAM_KEYS) { + if (body[key] !== undefined) { + captured[key] = sanitizeParamValue(body[key], 1); + } + } + } + return Object.keys(captured).length > 0 ? captured : undefined; } /** Extract the graph-determining fields from a request body for durable replay. */ diff --git a/packages/api/src/agents/load.spec.ts b/packages/api/src/agents/load.spec.ts new file mode 100644 index 0000000000..75abb6a3fd --- /dev/null +++ b/packages/api/src/agents/load.spec.ts @@ -0,0 +1,52 @@ +import type { LoadAgentDeps } from './load'; +import { loadEphemeralAgent } from './load'; + +const deps: LoadAgentDeps = { + getAgent: async () => null, + getMCPServerTools: async () => null, +}; + +const baseReq = { + user: { id: 'user-1' }, + config: { + modelSpecs: { list: [{ name: 'my-opus-spec', label: 'Spec Label' }] }, + }, + body: {}, +} as unknown as Parameters[0]['req']; + +async function idFor(modelParameters: Record) { + const agent = await loadEphemeralAgent( + { + req: baseReq, + spec: 'my-opus-spec', + endpoint: 'my-custom-endpoint', + model_parameters: modelParameters as never, + }, + deps, + ); + return agent?.id; +} + +/** + * Documents the #14253 Bug 2 mechanism: the ephemeral agent id (LangGraph node / + * HITL checkpoint namespace) is derived from `sender = modelLabel ?? modelSpec.label`. + * When the resume drops `modelLabel`, the id drifts and the paused checkpoint can't be + * re-entered. The fix keeps `modelLabel` across resume (RESUME_CONTEXT_KEYS), so the + * original and resumed ids stay equal. + */ +describe('loadEphemeralAgent ephemeral id stability (#14253 Bug 2)', () => { + test('id changes when modelLabel is lost vs preserved', async () => { + const withLabel = await idFor({ model: 'claude-opus-4', modelLabel: 'My Opus' }); + const withoutLabel = await idFor({ model: 'claude-opus-4' }); + expect(withLabel).toBeTruthy(); + expect(withoutLabel).toBeTruthy(); + // Original turn (has modelLabel) vs a resume that dropped it → different namespace. + expect(withLabel).not.toEqual(withoutLabel); + }); + + test('id is stable when modelLabel is preserved across turns', async () => { + const a = await idFor({ model: 'claude-opus-4', modelLabel: 'My Opus' }); + const b = await idFor({ model: 'claude-opus-4', modelLabel: 'My Opus' }); + expect(a).toEqual(b); + }); +});