mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🐛 refactor: anchor code-generated file lookup on threadFileIds for branched conversations (#13004)
* 🐛 fix: anchor getCodeGeneratedFiles on threadFileIds, not threadMessageIds In a branched conversation (regenerations producing the same code-output filename), `getCodeGeneratedFiles` would silently exclude files whose File-record `messageId` lived on a sibling branch. The user-visible symptom: "the previous file isn't persisted" — the LLM tries `load_workbook("output.xlsx")` on turn 2 and gets `FileNotFoundError` because LC sent `_injected_files: []` to codeapi instead of priming the prior turn's output. `claimCodeFile` is keyed by `(filename, conversationId, context)` — not by messageId. When sibling A first creates `output.csv`, the File record persists with `messageId = A`. When sibling N (a regeneration of A's parent) recreates `output.csv`, the claim finds A's record and `processCodeOutput` deliberately preserves `messageId = A` to keep file→original-creator provenance intact (correct behavior for the linear case where the original creator is in-thread). Turn N+1's `parentMessageId = N`. `getThreadData` walks back from N: the thread is `[N, root]` — sibling A is NOT in it. The pre-fix query filtered by `messageId IN [N, root]`, so the file was excluded. `getCodeGeneratedFiles` already lives next to `getUserCodeFiles`, which has always filtered by `file_id IN threadFileIds` (the file_ids referenced by `messages.files[]` arrays during the thread walk). The asymmetry — user-uploaded files anchored on the message's reference, code-generated files anchored on the File's own creator — was the bug. Anchoring both functions on `threadFileIds` reaches the right files regardless of which sibling first generated them. `File.messageId` stays informational ("who first generated this") for provenance and `processCodeOutput`'s "preserve original messageId on update" logic stays as-is — only the lookup key for thread-scoped fetches changes. - `packages/data-schemas/src/methods/file.ts`: signature + filter change. JSDoc spells out the branched-conversation rationale. - `packages/api/src/agents/initialize.ts`: pass `threadFileIds` instead of `threadMessageIds`. The local `threadMessageIds` declaration is removed since the only consumer is gone. - `packages/data-schemas/src/methods/file.spec.ts`: 5 new cases: - basic happy-path (file referenced by current thread) - **the regression**: file's creator messageId is on a sibling branch but file_id is in threadFileIds → finds it - empty/missing threadFileIds returns [] - cross-conversation isolation - non-execute_code context filter still applies (a chat attachment won't be returned even if its file_id is in threadFileIds — that's `getUserCodeFiles`'s job) Applies cleanly on top of dev. When LC #12960 (the typed CodeEnvRef cutover) lands, the only conflict is the legacy `metadata.fileIdentifier` metadata key flipping to `metadata.codeEnvRef` — same line, trivial resolve. - [x] `cd packages/data-schemas && npx jest src/methods/file.spec` — 42/42 pass (including the 5 new regression cases) - [x] `cd packages/api && npx jest src/agents` — 722/722 pass (modulo 2 pre-existing summarization e2e failures unrelated) - [x] `cd api && npx jest server/services/Files server/controllers/agents` — 432/432 pass - [x] `npx tsc --noEmit -p packages/api/tsconfig.json` — clean - [ ] Manual: branched conversation reproducer — generate a file in turn 1, regenerate the parent (sibling), then in turn N+1 ask the agent to read the file. Pre-fix: `FileNotFoundError`. Post-fix: the file is primed and load_workbook succeeds. * 🧪 test: lock initialize.ts → getCodeGeneratedFiles call shape Integration-level regression test asserting initializeAgent passes `threadFileIds` (not `threadMessageIds`) to getCodeGeneratedFiles in branched-conversation scenarios. Locks in the API shape from the previous commit, sitting one layer above the data-schemas unit test — so a future refactor to the priming chain can't silently revert to the messageId-based filter without surfacing a test failure here. Two cases: - The full call shape: agent.tools=['execute_code'], resendFiles=true, threadData mock returns distinct messageIds and fileIds. Asserts the call uses fileIds, and that getUserCodeFiles uses the same array (the symmetric design that closes the sibling-branch hole). - Empty threadFileIds: getCodeGeneratedFiles is still called with [] (its own internal early-return handles the empty case); getUserCodeFiles is gated at the call site and stays unscheduled.
This commit is contained in:
Danny Avila
parent
e7dbae32e5
commit
eb20d8805d
4 changed files with 329 additions and 23 deletions
|
|
@ -1231,3 +1231,130 @@ describe('initializeAgent — execute_code capability expansion', () => {
|
|||
).rejects.toThrow(/google_tool_conflict/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('initializeAgent — code-generated file thread filter (regression)', () => {
|
||||
/* Sibling-branched conversation regression. Pre-fix the priming chain
|
||||
* filtered code-generated files by `messageId IN threadMessageIds`,
|
||||
* which excluded files whose creator messageId lived on a sibling
|
||||
* branch (preserved on the File record by `processCodeOutput` for
|
||||
* provenance). The fix anchors `getCodeGeneratedFiles` on
|
||||
* `threadFileIds` instead — file_ids referenced by the thread's
|
||||
* `messages.files[]` arrays. This block locks the new contract at
|
||||
* the integration boundary: assert the right call shape, not the
|
||||
* underlying Mongo query (covered separately by
|
||||
* `data-schemas/methods/file.spec`). */
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
mockExtractLibreChatParams.mockReset();
|
||||
mockGetThreadData.mockReset();
|
||||
});
|
||||
|
||||
function setupExecuteCodeAgent() {
|
||||
const { agent, req, res, loadTools, db } = createMocks({
|
||||
provider: Providers.OPENAI,
|
||||
});
|
||||
agent.tools = ['execute_code'];
|
||||
|
||||
/* `resendFiles: true` is the gate that opens the thread-file
|
||||
* priming block in initialize.ts. Without it the whole
|
||||
* codeGeneratedFiles fetch is skipped. */
|
||||
mockExtractLibreChatParams.mockReturnValue({
|
||||
resendFiles: true,
|
||||
maxContextTokens: undefined,
|
||||
modelOptions: { model: 'test-model' },
|
||||
});
|
||||
|
||||
return { agent, req, res, loadTools, db };
|
||||
}
|
||||
|
||||
it('passes threadFileIds (not threadMessageIds) to getCodeGeneratedFiles', async () => {
|
||||
const { agent, req, res, loadTools, db } = setupExecuteCodeAgent();
|
||||
|
||||
/* Simulate the branched scenario: parent message N is a sibling
|
||||
* regeneration. `getThreadData` walks back from N and collects
|
||||
* messageIds [N, root] plus fileIds referenced by N.files[]. */
|
||||
mockGetThreadData.mockReturnValue({
|
||||
messageIds: ['msgN', 'msgRoot'],
|
||||
fileIds: ['file-pptx-skill', 'file-output-csv'],
|
||||
});
|
||||
|
||||
const getCodeGeneratedFiles = jest.fn().mockResolvedValue([]);
|
||||
const getUserCodeFiles = jest.fn().mockResolvedValue([]);
|
||||
const getMessages = jest
|
||||
.fn()
|
||||
.mockResolvedValue([{ messageId: 'msgN', parentMessageId: 'msgRoot', files: [] }]);
|
||||
|
||||
const dbWithThreadCalls: InitializeAgentDbMethods = {
|
||||
...db,
|
||||
getMessages,
|
||||
getCodeGeneratedFiles,
|
||||
getUserCodeFiles,
|
||||
};
|
||||
|
||||
await initializeAgent(
|
||||
{
|
||||
req,
|
||||
res,
|
||||
agent,
|
||||
loadTools,
|
||||
endpointOption: { endpoint: EModelEndpoint.agents },
|
||||
conversationId: 'conv-1',
|
||||
parentMessageId: 'msgN',
|
||||
allowedProviders: new Set([Providers.OPENAI]),
|
||||
isInitialAgent: true,
|
||||
codeEnvAvailable: true,
|
||||
},
|
||||
dbWithThreadCalls,
|
||||
);
|
||||
|
||||
expect(getCodeGeneratedFiles).toHaveBeenCalledTimes(1);
|
||||
expect(getCodeGeneratedFiles).toHaveBeenCalledWith('conv-1', [
|
||||
'file-pptx-skill',
|
||||
'file-output-csv',
|
||||
]);
|
||||
/* Both functions now share the same primary anchor — symmetric
|
||||
* design that closes the sibling-branch hole. */
|
||||
expect(getUserCodeFiles).toHaveBeenCalledWith(['file-pptx-skill', 'file-output-csv']);
|
||||
});
|
||||
|
||||
it('skips the code-generated fetch entirely when threadFileIds is empty', async () => {
|
||||
/* Empty `messages.files[]` across the thread — nothing to look up.
|
||||
* The function returns early without hitting Mongo, mirroring the
|
||||
* pre-fix behavior for empty-thread cases. */
|
||||
const { agent, req, res, loadTools, db } = setupExecuteCodeAgent();
|
||||
|
||||
mockGetThreadData.mockReturnValue({
|
||||
messageIds: ['msgN', 'msgRoot'],
|
||||
fileIds: [],
|
||||
});
|
||||
|
||||
const getCodeGeneratedFiles = jest.fn().mockResolvedValue([]);
|
||||
const getUserCodeFiles = jest.fn().mockResolvedValue([]);
|
||||
const getMessages = jest
|
||||
.fn()
|
||||
.mockResolvedValue([{ messageId: 'msgN', parentMessageId: 'msgRoot', files: [] }]);
|
||||
|
||||
await initializeAgent(
|
||||
{
|
||||
req,
|
||||
res,
|
||||
agent,
|
||||
loadTools,
|
||||
endpointOption: { endpoint: EModelEndpoint.agents },
|
||||
conversationId: 'conv-1',
|
||||
parentMessageId: 'msgN',
|
||||
allowedProviders: new Set([Providers.OPENAI]),
|
||||
isInitialAgent: true,
|
||||
codeEnvAvailable: true,
|
||||
},
|
||||
{ ...db, getMessages, getCodeGeneratedFiles, getUserCodeFiles },
|
||||
);
|
||||
|
||||
expect(getCodeGeneratedFiles).toHaveBeenCalledWith('conv-1', []);
|
||||
/* `getUserCodeFiles` is gated on a non-empty array at the call site,
|
||||
* so it shouldn't be invoked at all. `getCodeGeneratedFiles`'s own
|
||||
* empty-guard is exercised by data-schemas tests. */
|
||||
expect(getUserCodeFiles).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -233,8 +233,10 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods {
|
|||
getToolFilesByIds: (fileIds: string[], toolSet: Set<EToolResources>) => Promise<unknown[]>;
|
||||
/** Get conversation file IDs */
|
||||
getConvoFiles: (conversationId: string) => Promise<string[] | null>;
|
||||
/** Get code-generated files by conversation ID and optional message IDs */
|
||||
getCodeGeneratedFiles?: (conversationId: string, messageIds?: string[]) => Promise<unknown[]>;
|
||||
/** Get code-generated files by conversation ID and the file_ids
|
||||
* referenced from messages in the current thread (collected via
|
||||
* `messages.files[].file_id` during thread walk). */
|
||||
getCodeGeneratedFiles?: (conversationId: string, threadFileIds?: string[]) => Promise<unknown[]>;
|
||||
/** Get user-uploaded execute_code files by file IDs (from message.files in thread) */
|
||||
getUserCodeFiles?: (fileIds: string[]) => Promise<unknown[]>;
|
||||
/** Get messages for a conversation (supports select for field projection) */
|
||||
|
|
@ -423,7 +425,6 @@ export async function initializeAgent(
|
|||
let userCodeFiles: IMongoFile[] = [];
|
||||
|
||||
if (toolResourceSet.has(EToolResources.execute_code)) {
|
||||
let threadMessageIds: string[] | undefined;
|
||||
let threadFileIds: string[] | undefined;
|
||||
|
||||
if (parentMessageId && parentMessageId !== Constants.NO_PARENT && db.getMessages) {
|
||||
|
|
@ -433,22 +434,29 @@ export async function initializeAgent(
|
|||
'messageId parentMessageId files',
|
||||
);
|
||||
if (messages && messages.length > 0) {
|
||||
/** Single O(n) pass: build Map, traverse thread, collect both IDs */
|
||||
const threadData = getThreadData(messages, parentMessageId);
|
||||
threadMessageIds = threadData.messageIds;
|
||||
threadFileIds = threadData.fileIds;
|
||||
/** Walk the parent chain and collect file_ids referenced by
|
||||
* any message in the thread (`messages.files[].file_id`).
|
||||
* Used as the primary anchor for both
|
||||
* `getCodeGeneratedFiles` and `getUserCodeFiles` —
|
||||
* message ids no longer needed at this layer. */
|
||||
threadFileIds = getThreadData(messages, parentMessageId).fileIds;
|
||||
}
|
||||
}
|
||||
|
||||
/** Code-generated files (context: execute_code) filtered by messageId */
|
||||
/** Code-generated and user-uploaded execute_code files share the
|
||||
* same primary anchor: file_ids referenced by messages in the
|
||||
* current thread. The two queries differ only by `context`
|
||||
* (`execute_code` for generated outputs, others for uploads).
|
||||
* Anchoring both on `threadFileIds` reaches files regardless of
|
||||
* which sibling first generated them — see `getCodeGeneratedFiles`
|
||||
* for the branched-conversation rationale. */
|
||||
if (db.getCodeGeneratedFiles) {
|
||||
codeGeneratedFiles = (await db.getCodeGeneratedFiles(
|
||||
conversationId,
|
||||
threadMessageIds,
|
||||
threadFileIds,
|
||||
)) as IMongoFile[];
|
||||
}
|
||||
|
||||
/** User-uploaded execute_code files (context: agents/message_attachment) from thread messages */
|
||||
if (db.getUserCodeFiles && threadFileIds && threadFileIds.length > 0) {
|
||||
userCodeFiles = (await db.getUserCodeFiles(threadFileIds)) as IMongoFile[];
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue