mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: Codex round 3 + real Skip semantics
- Skip now ANSWERS instead of hiding UI (danny): it resumes the run with a
decline notice ('The user chose not to answer this question.') so the model
moves on — a client-side dismiss left the run paused until expiry, a hung
turn. × / Escape remain pure dismiss (switch to the inline card surface).
- P1 (resumed approval tool indices): resumed tool_calls steps whose
tool_call id matches a seeded UNRESOLVED part now rebind to that seeded
slot instead of offsetting — the original part resolves in place (output
attaches) and no duplicate appears; message steps keep the offset, so the
text-loss fix stands. createContentIndexOffsetHandlers now takes the seed
array; resolved seeded calls are not rebind targets.
- P2 (stale selection across questions): selection state resets when the
live actionId changes; the vestigial inline-Other state ('other' selection
+ text atom) is gone — the composer owns free-form.
- P2 (Redis abort path loses the args stamp): the abort route re-stamps the
question onto the ask tool_call in the reconstructed abort content, so a
Stop-abandoned question persists with its question intact.
- P2 (malformed args crash): parseAskUserQuestionArgs normalizes untrusted
shapes (options: {} / non-string entries) instead of throwing in render.
This commit is contained in:
parent
b215560d97
commit
7d7b0f0fc7
7 changed files with 161 additions and 60 deletions
|
|
@ -1920,7 +1920,7 @@ class AgentClient extends BaseClient {
|
|||
// type mismatch, is silently dropped against) the pre-pause content.
|
||||
customHandlers: createContentIndexOffsetHandlers(
|
||||
this.options.eventHandlers,
|
||||
Array.isArray(seedContent) ? seedContent.length : 0,
|
||||
Array.isArray(seedContent) ? seedContent : [],
|
||||
),
|
||||
requestBody: config.configurable.requestBody,
|
||||
user: createSafeUser(this.options.req?.user),
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ const {
|
|||
toClientPendingAction,
|
||||
isHITLEnabled,
|
||||
deleteAgentCheckpoint,
|
||||
attachAskUserQuestionArgs,
|
||||
} = require('@librechat/api');
|
||||
const { createSseStreamTelemetry } = require('@librechat/api/telemetry');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
|
|
@ -318,7 +319,15 @@ router.post('/chat/abort', configMiddleware, async (req, res) => {
|
|||
abortResult.jobData?.responseMessageId &&
|
||||
hasPersistableAbortContent(abortResult.content)
|
||||
) {
|
||||
const { jobData, content, text } = abortResult;
|
||||
const { jobData, text } = abortResult;
|
||||
let { content } = abortResult;
|
||||
// Redis reconstructs abort content from the graph/chunk log, which never saw
|
||||
// the pause-time args stamp applied to the in-process contentParts — re-stamp
|
||||
// here so a question abandoned via Stop persists with its question intact.
|
||||
const abortedPayload = job.metadata?.pendingAction?.payload;
|
||||
if (abortedPayload?.type === 'ask_user_question' && Array.isArray(content)) {
|
||||
content = attachAskUserQuestionArgs(content, abortedPayload.question);
|
||||
}
|
||||
const responseMessage = {
|
||||
messageId: jobData.responseMessageId,
|
||||
parentMessageId: jobData.userMessage.messageId,
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ import { cn } from '~/utils';
|
|||
*/
|
||||
function AskUserQuestionPopoverContent({ conversationId }: { conversationId: string }) {
|
||||
const localize = useLocalize();
|
||||
const { liveAsk, active, options, selected, setSelected, canSubmit, submit, dismiss } =
|
||||
const { liveAsk, active, options, selected, setSelected, canSubmit, submit, skip, dismiss } =
|
||||
useAskAnswerMode(conversationId);
|
||||
|
||||
if (!active || !liveAsk) {
|
||||
|
|
@ -61,7 +61,7 @@ function AskUserQuestionPopoverContent({ conversationId }: { conversationId: str
|
|||
</button>
|
||||
))}
|
||||
<div className="flex items-center justify-end gap-2 p-2">
|
||||
<Button size="sm" variant="outline" onClick={dismiss}>
|
||||
<Button size="sm" variant="outline" onClick={() => skip()}>
|
||||
{localize('com_ui_skip')}
|
||||
</Button>
|
||||
<Button size="sm" variant="submit" disabled={!canSubmit} onClick={() => submit()}>
|
||||
|
|
|
|||
|
|
@ -1,7 +1,11 @@
|
|||
import { useCallback, useMemo } from 'react';
|
||||
import { useCallback, useEffect, useMemo } from 'react';
|
||||
import { atom, useRecoilState } from 'recoil';
|
||||
import {
|
||||
ASK_USER_DECLINED_ANSWER,
|
||||
findLiveAskUserQuestion,
|
||||
splitOtherOption,
|
||||
} from '~/utils/approval';
|
||||
import { useResumeSubmit } from '~/components/Chat/Messages/Content/ApprovalContext';
|
||||
import { findLiveAskUserQuestion, splitOtherOption } from '~/utils/approval';
|
||||
import { useGetMessagesByConvoId } from '~/data-provider';
|
||||
|
||||
/** Dismissed action ids — recoil so every consumer reacts. */
|
||||
|
|
@ -10,18 +14,12 @@ const dismissedAskActionsAtom = atom<string[]>({
|
|||
default: [],
|
||||
});
|
||||
|
||||
/** Current selection: an option index, the inline free-form row, or nothing. */
|
||||
const askAnswerSelectionAtom = atom<number | 'other' | null>({
|
||||
/** Currently highlighted option row, or nothing. */
|
||||
const askAnswerSelectionAtom = atom<number | null>({
|
||||
key: 'askAnswerModeSelection',
|
||||
default: null,
|
||||
});
|
||||
|
||||
/** Text typed into the popover's inline "Other" input. */
|
||||
const askAnswerOtherTextAtom = atom<string>({
|
||||
key: 'askAnswerModeOtherText',
|
||||
default: '',
|
||||
});
|
||||
|
||||
/**
|
||||
* First-class "answer mode" for a live `ask_user_question` pause, with
|
||||
* select-then-confirm semantics (borrowed from Claude Code's AskUserQuestion
|
||||
|
|
@ -43,7 +41,6 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
);
|
||||
const [dismissedIds, setDismissedIds] = useRecoilState(dismissedAskActionsAtom);
|
||||
const [selected, setSelected] = useRecoilState(askAnswerSelectionAtom);
|
||||
const [otherText, setOtherText] = useRecoilState(askAnswerOtherTextAtom);
|
||||
const { submitAskAnswer } = useResumeSubmit();
|
||||
|
||||
const dismissed = liveAsk != null && dismissedIds.includes(liveAsk.actionId);
|
||||
|
|
@ -53,6 +50,12 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
[liveAsk],
|
||||
);
|
||||
|
||||
/** Selection is per-question: a new pause must never inherit a stale
|
||||
* highlight whose Enter would submit the previous question's choice. */
|
||||
useEffect(() => {
|
||||
setSelected(null);
|
||||
}, [liveAsk?.actionId, setSelected]);
|
||||
|
||||
const dismiss = useCallback(() => {
|
||||
if (liveAsk) {
|
||||
setDismissedIds((prev) =>
|
||||
|
|
@ -61,32 +64,17 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
}
|
||||
}, [liveAsk, setDismissedIds]);
|
||||
|
||||
const canSubmit =
|
||||
active &&
|
||||
(typeof selected === 'number'
|
||||
? options[selected] != null
|
||||
: selected === 'other' && otherText.trim().length > 0);
|
||||
const canSubmit = active && typeof selected === 'number' && options[selected] != null;
|
||||
|
||||
/** Fires the confirmed selection; returns true when an answer was sent. */
|
||||
const submit = useCallback((): boolean => {
|
||||
if (!liveAsk || !canSubmit) {
|
||||
if (!liveAsk || !canSubmit || typeof selected !== 'number') {
|
||||
return false;
|
||||
}
|
||||
const answer = typeof selected === 'number' ? options[selected].value : otherText.trim();
|
||||
submitAskAnswer(liveAsk.actionId, answer);
|
||||
submitAskAnswer(liveAsk.actionId, options[selected].value);
|
||||
setSelected(null);
|
||||
setOtherText('');
|
||||
return true;
|
||||
}, [
|
||||
liveAsk,
|
||||
canSubmit,
|
||||
selected,
|
||||
options,
|
||||
otherText,
|
||||
submitAskAnswer,
|
||||
setSelected,
|
||||
setOtherText,
|
||||
]);
|
||||
}, [liveAsk, canSubmit, selected, options, submitAskAnswer, setSelected]);
|
||||
|
||||
/** Composer text answers the question directly; true when consumed. */
|
||||
const submitText = useCallback(
|
||||
|
|
@ -98,13 +86,26 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
if (trimmed.length > 0) {
|
||||
submitAskAnswer(liveAsk.actionId, trimmed);
|
||||
setSelected(null);
|
||||
setOtherText('');
|
||||
}
|
||||
return true;
|
||||
},
|
||||
[active, liveAsk, submitAskAnswer, setSelected, setOtherText],
|
||||
[active, liveAsk, submitAskAnswer, setSelected],
|
||||
);
|
||||
|
||||
/**
|
||||
* Explicitly decline: resumes the run with a canned notice so the model
|
||||
* knows the user chose not to answer. A client-side dismiss alone would
|
||||
* leave the run paused until expiry — a hung turn.
|
||||
*/
|
||||
const skip = useCallback((): boolean => {
|
||||
if (!active || !liveAsk) {
|
||||
return false;
|
||||
}
|
||||
submitAskAnswer(liveAsk.actionId, ASK_USER_DECLINED_ANSWER);
|
||||
setSelected(null);
|
||||
return true;
|
||||
}, [active, liveAsk, submitAskAnswer, setSelected]);
|
||||
|
||||
/** Selection steering from the empty composer; true when consumed. */
|
||||
const handleComposerKeyDown = useCallback(
|
||||
(e: React.KeyboardEvent<HTMLTextAreaElement>): boolean => {
|
||||
|
|
@ -121,25 +122,27 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
}
|
||||
return false;
|
||||
}
|
||||
const rowCount = options.length + 1; // + the inline Other row
|
||||
const asIndex = (value: number | 'other' | null): number =>
|
||||
value === 'other' ? options.length : (value ?? -1);
|
||||
const fromIndex = (index: number): number | 'other' =>
|
||||
index >= options.length ? 'other' : index;
|
||||
if (options.length === 0) {
|
||||
if (e.key === 'Escape') {
|
||||
dismiss();
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
const digit = Number.parseInt(e.key, 10);
|
||||
if (!Number.isNaN(digit) && digit >= 1 && digit <= Math.min(rowCount, 9)) {
|
||||
if (!Number.isNaN(digit) && digit >= 1 && digit <= Math.min(options.length, 9)) {
|
||||
e.preventDefault();
|
||||
setSelected(fromIndex(digit - 1));
|
||||
setSelected(digit - 1);
|
||||
return true;
|
||||
}
|
||||
if (e.key === 'ArrowDown') {
|
||||
e.preventDefault();
|
||||
setSelected(fromIndex((asIndex(selected) + 1) % rowCount));
|
||||
setSelected(((selected ?? -1) + 1) % options.length);
|
||||
return true;
|
||||
}
|
||||
if (e.key === 'ArrowUp') {
|
||||
e.preventDefault();
|
||||
setSelected(fromIndex((asIndex(selected) - 1 + rowCount) % rowCount));
|
||||
setSelected(((selected ?? 0) - 1 + options.length) % options.length);
|
||||
return true;
|
||||
}
|
||||
if (e.key === 'Enter' && !e.shiftKey && canSubmit) {
|
||||
|
|
@ -164,11 +167,10 @@ export default function useAskAnswerMode(conversationId?: string | null) {
|
|||
dismiss,
|
||||
selected,
|
||||
setSelected,
|
||||
otherText,
|
||||
setOtherText,
|
||||
canSubmit,
|
||||
submit,
|
||||
submitText,
|
||||
skip,
|
||||
handleComposerKeyDown,
|
||||
/** Model-supplied "Other"-style label, folded into the inline input. */
|
||||
otherLabel,
|
||||
|
|
|
|||
|
|
@ -10,6 +10,13 @@ import type { Agents, TMessage, TMessageContentParts } from 'librechat-data-prov
|
|||
*/
|
||||
export const ASK_USER_QUESTION = 'ask_user_question' as const;
|
||||
|
||||
/**
|
||||
* Answer sent when the user explicitly skips a question: the run must resume
|
||||
* (a client-side dismiss would leave it paused until expiry — a hung turn),
|
||||
* and the model needs to know the user declined rather than answered.
|
||||
*/
|
||||
export const ASK_USER_DECLINED_ANSWER = 'The user chose not to answer this question.';
|
||||
|
||||
/** Shape of the synthetic content part carrying an ask-user pending action. */
|
||||
export interface AskUserQuestionPart {
|
||||
type: typeof ASK_USER_QUESTION;
|
||||
|
|
@ -203,7 +210,22 @@ export function parseAskUserQuestionArgs(
|
|||
) {
|
||||
return null;
|
||||
}
|
||||
return parsed as unknown as Agents.AskUserQuestionRequest;
|
||||
const request = parsed as { question: string; description?: unknown; options?: unknown };
|
||||
/** Model/persisted args are untrusted — normalize instead of crashing the
|
||||
* message render on shapes like `options: {}` or non-string entries. */
|
||||
const options = Array.isArray(request.options)
|
||||
? request.options.filter(
|
||||
(option): option is Agents.AskUserQuestionOption =>
|
||||
option != null &&
|
||||
typeof (option as { label?: unknown }).label === 'string' &&
|
||||
typeof (option as { value?: unknown }).value === 'string',
|
||||
)
|
||||
: undefined;
|
||||
return {
|
||||
question: request.question,
|
||||
description: typeof request.description === 'string' ? request.description : undefined,
|
||||
options: options && options.length > 0 ? options : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -174,6 +174,7 @@ describe('findIncompleteDecisions', () => {
|
|||
});
|
||||
|
||||
describe('createContentIndexOffsetHandlers', () => {
|
||||
const textSeed = (n: number) => Array.from({ length: n }, () => ({ type: 'text' }));
|
||||
const makeRecorder = () => {
|
||||
const calls: Array<{ event: string; data: unknown }> = [];
|
||||
const handler = { handle: (event: string, data: unknown) => void calls.push({ event, data }) };
|
||||
|
|
@ -183,13 +184,13 @@ describe('createContentIndexOffsetHandlers', () => {
|
|||
it('returns the input untouched for offset 0 / undefined handlers', () => {
|
||||
const { handler } = makeRecorder();
|
||||
const handlers = { on_run_step: handler };
|
||||
expect(createContentIndexOffsetHandlers(handlers, 0)).toBe(handlers);
|
||||
expect(createContentIndexOffsetHandlers(undefined, 3)).toBeUndefined();
|
||||
expect(createContentIndexOffsetHandlers(handlers, [])).toBe(handlers);
|
||||
expect(createContentIndexOffsetHandlers(undefined, textSeed(3))).toBeUndefined();
|
||||
});
|
||||
|
||||
it('shifts ON_RUN_STEP index by the offset without mutating the original payload', () => {
|
||||
const { calls, handler } = makeRecorder();
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_run_step: handler }, 3)!;
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_run_step: handler }, textSeed(3))!;
|
||||
const runStep = { id: 'step_1', index: 0, stepDetails: { type: 'message_creation' } };
|
||||
wrapped.on_run_step.handle('on_run_step', runStep as never);
|
||||
expect((calls[0].data as { index: number }).index).toBe(3);
|
||||
|
|
@ -198,7 +199,7 @@ describe('createContentIndexOffsetHandlers', () => {
|
|||
|
||||
it('shifts ON_AGENT_UPDATE nested index', () => {
|
||||
const { calls, handler } = makeRecorder();
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_agent_update: handler }, 2)!;
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_agent_update: handler }, textSeed(2))!;
|
||||
wrapped.on_agent_update.handle('on_agent_update', {
|
||||
agent_update: { index: 1, runId: 'r' },
|
||||
} as never);
|
||||
|
|
@ -210,7 +211,7 @@ describe('createContentIndexOffsetHandlers', () => {
|
|||
const deltaHandler = { handle: jest.fn() };
|
||||
const wrapped = createContentIndexOffsetHandlers(
|
||||
{ on_run_step: handler, on_message_delta: deltaHandler },
|
||||
5,
|
||||
textSeed(5),
|
||||
)!;
|
||||
expect(wrapped.on_message_delta).toBe(deltaHandler);
|
||||
// Deltas carry no index — they resolve through the (shifted) stepMap entry.
|
||||
|
|
@ -218,9 +219,43 @@ describe('createContentIndexOffsetHandlers', () => {
|
|||
expect(deltaHandler.handle).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('rebinds a resumed tool step to its seeded unresolved slot by tool_call id', () => {
|
||||
const { calls, handler } = makeRecorder();
|
||||
const seed = [
|
||||
{ type: 'text' },
|
||||
{ type: 'tool_call', tool_call: { id: 'tc_paused', output: '' } },
|
||||
{ type: 'tool_call', tool_call: { id: 'tc_done', output: 'already resolved' } },
|
||||
];
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_run_step: handler }, seed)!;
|
||||
|
||||
// The paused call's re-execution must land on its seeded slot (1), not 0+3.
|
||||
wrapped.on_run_step.handle('on_run_step', {
|
||||
id: 'step_t',
|
||||
index: 0,
|
||||
stepDetails: { type: 'tool_calls', tool_calls: [{ id: 'tc_paused' }] },
|
||||
} as never);
|
||||
expect((calls[0].data as { index: number }).index).toBe(1);
|
||||
|
||||
// A resolved seeded call is NOT a rebind target — a fresh same-id step offsets.
|
||||
wrapped.on_run_step.handle('on_run_step', {
|
||||
id: 'step_u',
|
||||
index: 1,
|
||||
stepDetails: { type: 'tool_calls', tool_calls: [{ id: 'tc_done' }] },
|
||||
} as never);
|
||||
expect((calls[1].data as { index: number }).index).toBe(4);
|
||||
|
||||
// Message steps always offset.
|
||||
wrapped.on_run_step.handle('on_run_step', {
|
||||
id: 'step_m',
|
||||
index: 2,
|
||||
stepDetails: { type: 'message_creation' },
|
||||
} as never);
|
||||
expect((calls[2].data as { index: number }).index).toBe(5);
|
||||
});
|
||||
|
||||
it('leaves a runStep without a numeric index unshifted (defensive)', () => {
|
||||
const { calls, handler } = makeRecorder();
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_run_step: handler }, 4)!;
|
||||
const wrapped = createContentIndexOffsetHandlers({ on_run_step: handler }, textSeed(4))!;
|
||||
const weird = { id: 'step_x' };
|
||||
wrapped.on_run_step.handle('on_run_step', weird as never);
|
||||
expect(calls[0].data).toBe(weird);
|
||||
|
|
|
|||
|
|
@ -143,23 +143,56 @@ export function findIncompleteDecisions(
|
|||
*/
|
||||
export function createContentIndexOffsetHandlers(
|
||||
handlers: Record<string, EventHandler> | undefined,
|
||||
offset: number,
|
||||
seedContent: Array<{ type?: string; tool_call?: { id?: string; output?: unknown } }> = [],
|
||||
): Record<string, EventHandler> | undefined {
|
||||
const offset = seedContent.length;
|
||||
if (handlers == null || !(offset > 0)) {
|
||||
return handlers;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resumed tool steps for calls the PAUSED turn already rendered must land
|
||||
* back on their seeded slot — not a fresh offset slot — or the original
|
||||
* part stays unresolved while a duplicate completed one appears after the
|
||||
* seed (and its output never attaches). Map unresolved seeded tool_calls by
|
||||
* id so the resume pass's re-execution (approval flows re-run the approved
|
||||
* tool; ask re-runs its body) rebinds to the right index.
|
||||
*/
|
||||
const seededToolCallIndex = new Map<string, number>();
|
||||
seedContent.forEach((part, index) => {
|
||||
const toolCall = part?.tool_call;
|
||||
const unresolved =
|
||||
typeof toolCall?.output !== 'string' || (toolCall.output as string).length === 0;
|
||||
if (part?.type === 'tool_call' && typeof toolCall?.id === 'string' && unresolved) {
|
||||
seededToolCallIndex.set(toolCall.id, index);
|
||||
}
|
||||
});
|
||||
|
||||
const wrapped: Record<string, EventHandler> = { ...handlers };
|
||||
|
||||
const runStepHandler = handlers[GraphEvents.ON_RUN_STEP];
|
||||
if (runStepHandler) {
|
||||
wrapped[GraphEvents.ON_RUN_STEP] = {
|
||||
handle: (event, data, metadata, graph) => {
|
||||
const runStep = data as { index?: number } | undefined;
|
||||
const shifted =
|
||||
runStep != null && typeof runStep.index === 'number'
|
||||
? { ...runStep, index: runStep.index + offset }
|
||||
: data;
|
||||
const runStep = data as
|
||||
| {
|
||||
index?: number;
|
||||
stepDetails?: { type?: string; tool_calls?: Array<{ id?: string }> };
|
||||
}
|
||||
| undefined;
|
||||
if (runStep == null || typeof runStep.index !== 'number') {
|
||||
return runStepHandler.handle(event, data, metadata, graph);
|
||||
}
|
||||
const seededIndex =
|
||||
runStep.stepDetails?.type === 'tool_calls'
|
||||
? runStep.stepDetails.tool_calls
|
||||
?.map((call) => (call.id ? seededToolCallIndex.get(call.id) : undefined))
|
||||
.find((index) => index != null)
|
||||
: undefined;
|
||||
const shifted = {
|
||||
...runStep,
|
||||
index: seededIndex ?? runStep.index + offset,
|
||||
};
|
||||
return runStepHandler.handle(event, shifted as typeof data, metadata, graph);
|
||||
},
|
||||
};
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue