🧵 feat: Native Background Execution for Code Interpreter Tools - #14386
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Enables native background execution for the Code Interpreter tool pair (execute_code / runtime bash_tool) on the existing background tool-call substrate, including post-completion harvesting that anchors stdout + generated files back onto the original dispatch tool call (and preserves sandbox session continuity across polls).
Changes:
- Makes
execute_code/bash_tooleligible for background dispatch and expands opt-ins so either tool key enables the pair. - Adds completion-time code harvest + persistence: patches the original tool-call output, persists generated files, appends attachments, and re-emits/re-anchors on polls.
- Updates client UX: adds an agent-builder “Background execution” toggle for code tools and renders “Running/Finished in background” instead of showing the handle JSON as stdout.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/methods/message.ts | Adds updateToolCallResult to patch tool-call output and append/dedupe attachments atomically. |
| packages/data-schemas/src/methods/message.spec.ts | Adds mongodb-memory-server coverage for updateToolCallResult behaviors (patching, idempotency, isolation). |
| packages/api/src/agents/run.ts | Registers check_background_task as a code-session participant for session folding on polls. |
| packages/api/src/agents/handlers.ts | Threads full code-session config into detached invokes; adds harvest + poll-time re-emit/re-anchor logic. |
| packages/api/src/agents/handlers.background.spec.ts | Adds suite validating backgrounded code execution (config carryover, harvest anchoring, poll behavior, fallback). |
| packages/api/src/agents/background.ts | Removes code tools from exclusion set; adds code-pair opt-in expansion and harvest state tracking/delivery helpers. |
| packages/api/src/agents/background.spec.ts | Updates eligibility expectations; adds tests for code-pair expansion and harvest delivery surface. |
| packages/api/src/agents/tests/load.spec.ts | Updates background tool option synthesis expectations to include execute_code. |
| client/src/locales/en/translation.json | Adds strings for code background toggle + background status labels. |
| client/src/hooks/Messages/useAttachments.ts | Merges live-only SSE attachments with DB snapshot to avoid hiding harvested files pre-reload. |
| client/src/hooks/Messages/tests/useAttachments.spec.tsx | Updates attachment merge contract test for live-only entries. |
| client/src/hooks/Agents/useMCPToolOptions.ts | Exports withBooleanOption for reuse by built-in tool option UI. |
| client/src/components/SidePanel/Agents/Tools/ItemDialog/sections/BuiltinSection.tsx | Adds code background toggle UI alongside Code Interpreter config. |
| client/src/components/SidePanel/Agents/Code/Background.tsx | New: agent-builder “Background execution” switch writing tool_options for both code tools. |
| client/src/components/Chat/Messages/Content/Parts/handle.ts | New: parseBackgroundHandle helper to detect dispatch handles. |
| client/src/components/Chat/Messages/Content/Parts/ExecuteCode.tsx | Renders background running/finished state instead of handle JSON stdout. |
| client/src/components/Chat/Messages/Content/Parts/BashCall.tsx | Same background-state rendering behavior for bash calls. |
| client/src/components/Chat/Messages/Content/Parts/tests/handle.test.ts | New: unit tests for parseBackgroundHandle. |
| client/src/components/Chat/Messages/Content/Parts/tests/BashCall.test.tsx | Adds tests covering bash background states and stdout rendering after patch. |
| api/server/services/Endpoints/agents/initialize.js | Wires background code persister + attachment emitter into agent execution deps. |
| api/server/controllers/agents/callbacks.js | Adds completion-time harvest handler + poll-stream attachment emitter and exports them. |
| api/server/controllers/agents/callbacks.background.spec.js | New: tests for harvest handler (persistence, retries, reapply mode, error tolerance). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isCodeCall = isCodeSessionAwareToolCall(tc.name, mergedConfigurable); | ||
| const harvestEnabled = isCodeCall && persistBackgroundCodeResult != null; | ||
| /** Persists the settled result onto the dispatch turn's message |
| if (attachments !== undefined && attachments.length > 0) { | ||
| const fileIds = attachments | ||
| .map((attachment) => (attachment as { file_id?: unknown }).file_id) | ||
| .filter((id): id is string => typeof id === 'string'); | ||
| stages.push({ | ||
| $set: { | ||
| attachments: { | ||
| $concatArrays: [ | ||
| { | ||
| $filter: { | ||
| input: { $ifNull: ['$attachments', []] }, | ||
| as: 'existing', | ||
| cond: { $not: [{ $in: ['$$existing.file_id', { $literal: fileIds }] }] }, | ||
| }, | ||
| }, | ||
| { $literal: attachments }, | ||
| ], | ||
| }, | ||
| }, | ||
| }); | ||
| } |
| if (stages.length === 0) { | ||
| return false; | ||
| } |
|
@codex review |
f4b1e3c to
425c07c
Compare
![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: 425c07ce45
ℹ️ 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".
| cond: { $not: [{ $in: ['$$existing.file_id', { $literal: fileIds }] }] }, | ||
| }, | ||
| }, | ||
| { $literal: attachments }, |
There was a problem hiding this comment.
When a background code artifact is saved through createDownloadFallback (oversized file, missing storage strategy, or download failure), the attachment object has no file_id. On every specific check_background_task poll this patch is re-applied with the retained attachments, but the dedupe filter only removes existing rows whose file_id is in fileIds, then appends the entire attachment list again. For fallback attachments fileIds is empty (or doesn't include that object), so each poll duplicates the same attachment in the message row and in the UI; either avoid reapplying no-file_id attachments or dedupe them by another stable key such as filepath/toolCallId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee16ea7 — the pipeline dedupe key is now file_id ?? filepath (mirroring the resume merge's key), so download-fallback attachments without a file_id are deduped by filepath across re-applications. Added a spec re-applying a filepath-only attachment twice and asserting no duplicates.
| /** Harvested code tasks never route through the poll turn's | ||
| * callback — their files were already persisted with the | ||
| * ORIGINAL tool-call identity by the completion harvest. */ | ||
| if (toolEndCallback && !(isCodeTask && pending.harvestStarted === true)) { |
There was a problem hiding this comment.
When persistBackgroundCodeResult rejects or returns null before attachHarvest records attachments, this condition still suppresses the normal poll-turn toolEndCallback solely because harvestStarted was set when the task completed. The artifact has just been claimed and cleared, so generated files from that background code run are never processed or attached even though the fallback path would have handled them; track harvest success/failure and only skip the callback after the harvest actually persisted the files, or restore the artifact for a later fallback poll.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee16ea7 — added revokeHarvest to the registry: when the harvest rejects (or returns null for a missing anchor identity) it clears harvestStarted and restores the artifact if a poll already claimed it mid-flight, so the next poll takes the legacy toolEndCallback delivery path and the files are processed. Covered by a spec where the persister throws and the poll still delivers the artifact via the callback.
| attachments && attachments.length > 0 | ||
| ? 'com_ui_background_finished' | ||
| : 'com_ui_background_running', |
There was a problem hiding this comment.
For a backgrounded code execution that completes with stdout/stderr but produces no files, attachments remains empty forever, and the DB-only output patch does not update the already-rendered message in the current React state. After the model polls and receives a completed result, the original code card can still say Running in background until a full reload; use an explicit completed status/result for the handle rather than treating attachment arrival as the only completion signal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — this is the documented known limit from the PR description (live label for stdout-only tasks until reload). Attachment arrival is the only client-side completion signal that exists today; a real fix needs a content-part update SSE channel so the server can patch an already-rendered tool_call part in place, which is a new event type and client handler. Keeping that out of this PR's scope and tracking it as the follow-up alongside durable (Redis) task storage — the DB patch already makes reload and later turns render correctly.
| const { backgroundToolsEnabled } = useAgentCapabilities(agentsConfig?.capabilities); | ||
| const { control, getValues, setValue } = useFormContext<AgentForm>(); | ||
| const toolOptions = useWatch({ control, name: 'tool_options' }); | ||
| const enabled = toolOptions?.[Tools.execute_code]?.run_in_background === true; |
There was a problem hiding this comment.
The backend treats either execute_code or bash_tool as enabling the paired code background option, but the builder switch reads only tool_options.execute_code. An agent created through the API or an older/manual edit with only tool_options.bash_tool.run_in_background: true will show this switch off while background execution is actually enabled, and the first click turns it on again instead of disabling it; compute enabled from both code tool ids.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee16ea7 — the switch now computes enabled from both code tool ids (execute_code OR bash_tool), matching the backend pair expansion, so a bash_tool-only opt-in renders as on and the first click disables it.
| if ( | ||
| parsed != null && | ||
| typeof parsed.background_task_id === 'string' && | ||
| typeof parsed.tool === 'string' | ||
| ) { |
There was a problem hiding this comment.
If user code intentionally prints a small JSON object that contains string background_task_id and tool fields, this parser treats that real stdout as a synthetic dispatch handle and the code card hides it behind the background status text. That can happen after the background result has been patched to real output, not only during dispatch; require a stronger marker from buildBackgroundHandleContent (for example the expected status/message shape) before suppressing stdout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee16ea7 — parseBackgroundHandle now requires the handle's exact shape: exactly the four keys (background_task_id, tool, status, message), all strings, and message containing the check_background_task poll instruction that buildBackgroundHandleContent always embeds. Added negative specs for mimicking stdout and extra-key payloads.
| if ( | ||
| delivery && | ||
| delivery.status === 'completed' && | ||
| isCodeSessionAwareToolCall(delivery.toolName, mergedConfigurable) |
There was a problem hiding this comment.
The reapply/heal path only runs for completed deliveries, so a background code task that fails is never re-anchored on later polls after a full-row save reverts the original error-output patch. In HITL pause/resume or similar rewrite flows, the original card can go back to the dispatch handle even though check_background_task reports error; include harvested error tasks in the reapply path so failed code outputs get the same idempotent repair as successful ones.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee16ea7 — fail() now records harvestStarted for code tasks, the delivery view exposes the task's error, and the poll heal path runs for any settled status with output = result ?? error — so a reverted error patch is re-anchored the same way a success is. Covered by a spec asserting the reapply call carries the error output.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee16ea7fce
ℹ️ 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".
| attachments && attachments.length > 0 | ||
| ? 'com_ui_background_finished' | ||
| : 'com_ui_background_running', |
There was a problem hiding this comment.
When a backgrounded execute_code/bash_tool finishes without generating files, the backend only patches Mongo and poll delivery emits no attachments, so the in-memory message still has the original background handle and an empty attachments array. This branch therefore keeps rendering “Running in background” until a full reload, even though the task completed and the poll result contains stdout. Please add a live completion/content update signal for no-file completions instead of using attachment presence as the only finished signal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6535b57 — you were right to push on this; there was a contained live path after all. Settled code tasks now emit a synthetic background_task_status attachment on the poll turn over the EXISTING attachment SSE channel (stable id bg-<toolCallId>, upserted client-side, never persisted, filtered out of file rendering). The card treats the marker as the completion signal, so stdout-only runs flip to "Finished in background" live; the patched output still renders natively on reload. No new event type needed.
| * those files vanish until a full reload. */ | ||
| const liveOnly = live.filter((a) => { | ||
| const id = (a as Partial<TFile>).file_id; | ||
| return !id || !dbFileIds.has(id); |
There was a problem hiding this comment.
When a tool artifact has no file_id (for example file_search attachments), the SSE handler stores it in messageAttachmentsMap, and finalHandler later replays the same responseMessage.attachments while the message also carries those persisted attachments. This predicate treats every unkeyed live entry as live-only, so after final render useAttachments appends duplicates to the DB list; file-search cards then process the same sources twice. Keep only live entries with a stable absent key, or dedupe unkeyed attachments by type/toolCallId, instead of accepting all !id entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6535b57 — the live-only merge now requires a stable identity: file_id ?? filepath ?? type:toolCallId. Unkeyed live entries whose composite key already appears in the DB list (the replayed file_search citation case) are dropped as duplicates, and entries with no identity at all are dropped entirely (restoring the old behavior for them). Added specs for both cases.
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6dde295b5
ℹ️ 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".
| ); | ||
| return { | ||
| fileAttachments, | ||
| backgroundSettled: fileAttachments.length !== attachments.length, |
There was a problem hiding this comment.
When a backgrounded code task fails, the server emits this synthetic marker with status: 'error', but this helper collapses every marker into backgroundSettled: true. BashCall/ExecuteCode then keep the original handle JSON hidden and render Finished in background with no error state on the original card after the user polls a failed run, until a reload or other refetch replaces the output. Please preserve the marker status (or at least an error boolean) instead of treating all markers as successful completion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6a5287 — splitBackgroundAttachments now returns the marker's status instead of collapsing to a boolean; ExecuteCode/BashCall render the standard tool failed error suffix when a polled background run settled with status: 'error'. Added a card spec for the error marker.
| continue; | ||
| } | ||
| try { | ||
| const result = await processCodeOutput({ |
There was a problem hiding this comment.
When a background code run writes a common filename (for example output.csv or plot.png) and a later code run in the same conversation writes that filename before this detached harvest reaches processCodeOutput, the existing file-claim path reuses the conversation/filename File row and createFile upserts by that file_id. That lets the older background completion overwrite the newer run’s file contents/metadata and then dedupe attachments by the same file_id, so users and later code priming can see stale artifacts. Please add an ordering/tool-call guard or avoid reusing an existing file claim for out-of-order background harvests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6a5287 — added an out-of-order guard to processCodeOutput: the background harvest passes freshClaimAfter (harvest start time), and when the claimed row's updatedAt is newer, the harvest writes under a fresh file_id instead of reusing the claim — the newer run's content, metadata, and attachment identity are untouched, and the stale output still persists as its own row anchored to its own message. Covered by specs for both the guarded and pass-through cases.
| } | ||
|
|
||
| const BACKGROUND_PATCH_RETRY_DELAYS_MS = [ | ||
| 2_000, 5_000, 10_000, 20_000, 30_000, 60_000, 120_000, 180_000, 240_000, 300_000, |
There was a problem hiding this comment.
If a fast background code run settles before the dispatch assistant message is persisted, the first patch attempt returns false and this schedule waits at least 2 seconds (or 5 seconds after a second miss) before trying again. A user/model follow-up sent as soon as the prior turn finalizes can initialize during that gap, before the generated attachments have been appended to the message, so thread file collection has no file_ids to prime and the new code turn cannot see the completed background outputs. Please add an immediate/short retry around the expected turn-finalization window before backing off.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6a5287 — the anchor retry schedule now leads with sub-second attempts (250ms, 500ms, 1s, 2s…) before backing off, so a fast background completion racing the dispatch turn's finalization is anchored within the turn-boundary window and an immediate follow-up turn primes the generated files. Updated the fake-timer spec to the new schedule.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6a5287a17
ℹ️ 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".
| file_id = newFileId; | ||
| isUpdate = false; |
There was a problem hiding this comment.
When a background harvest sees an existing code-output filename, claimCodeFile is a findOneAndUpdate on a timestamped File schema, so the returned claim can have updatedAt later than freshClaimAfter even for the harvest's own claim attempt. This branch then switches to newFileId, but the File schema still has a unique index on (filename, conversationId, context, tenantId), so the subsequent createFile upsert by that fresh file_id tries to insert a duplicate filename slot and the attachment is dropped. This breaks common background code runs that overwrite/regenerate output.csv or similar files already present in the conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8384d — both halves were right. (1) claimCodeFile now passes timestamps: false (a claim is an id reservation, not a content write), so updatedAt reflects the last real write and the guard no longer misfires on the harvest's own claim. (2) The conflict path no longer mints a fresh file_id (the (filename, conversationId) unique index forbids it) — a genuinely stale harvest now SKIPS the file with a warn, so the newer run's content and attachment identity win. Regenerating existing filenames in background works normally again; verified with a real-Mongo timestamp test plus updated guard specs.
| * the dispatch card on the handle JSON forever. */ | ||
| { harvestStarted: harvestEnabled }, | ||
| ); | ||
| harvestCodeResult({ output: message }); |
There was a problem hiding this comment.
If the detached code invocation throws with a normal message such as Command failed with exit code 1, this persists only the raw message onto the original tool-call output while the dispatch tool call remains a successful background handle. After reload, the non-persisted status-marker attachment is gone and the code cards only detect errors from specific Error: prefixes, so these failed background executions render as clean completions with ordinary stdout. The persisted patch needs to carry an error status or use the same error output shape as foreground tool failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8384d — detached code failures are wrapped to the foreground CodeExecutor error shape (Execution error:\n\n<message>) unless the message already carries a recognizable error prefix, at both the registry (poll result) and the patched part output, so a reloaded card and later model turns read the failure as an error rather than clean stdout.
| if (!task || task.harvestStarted !== true) { | ||
| return undefined; |
There was a problem hiding this comment.
When a backgrounded code invocation never settles and the registry reaps it as Background task timed out, no harvest ever runs, so harvestStarted remains unset. This guard then suppresses the synthetic status marker and re-anchor path even though runCheckBackgroundTask reports an error, leaving the original code card stuck on “Running in background” indefinitely after the user polls the timed-out task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8384d — the harvest flag is now armed at DISPATCH (create({ harvestStarted })) rather than at settle, so a task the registry reaps as timed out still takes the marker + re-anchor path: polls emit the error status marker and heal the dispatch card with Background task timed out instead of leaving it on "Running in background" forever. Covered by a fake-timer spec advancing past the running TTL.
| function createBackgroundCodeResultHandler({ req, updateToolCallResult }) { | ||
| const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
For this new code path, createBackgroundCodeResultHandler is substantial backend logic in /api/server/controllers/agents/callbacks.js; the root AGENTS instructions delegate to CLAUDE.md, which says new backend code belongs in /packages/api TypeScript and /api should stay thin. Keeping the retry/persistence/file-harvest implementation here bypasses the TS workspace and entrenches the legacy controller, so please move the implementation to packages/api and leave this file as a thin wrapper.
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb8384d — fair call. The harvest implementation (retry schedule, file iteration, reapply mode) now lives in TypeScript at packages/api/src/agents/harvest.ts with typed dependency injection for the host file services; api/server/controllers/agents/callbacks.js keeps only a thin wrapper binding processCodeOutput/runPreviewFinalize into it. The existing wrapper-level spec still covers the behavior end-to-end.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8384dce6
ℹ️ 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".
| for (const attachment of delivery.attachments ?? []) { | ||
| try { | ||
| emitAttachment?.(attachment); |
There was a problem hiding this comment.
When a generated code file falls back to a download URL (for example because it exceeds the size limit or storage is unavailable), processCodeOutput returns an attachment with a filepath but no file_id. This poll-time loop re-emits every harvested attachment on each check_background_task call, but the SSE attachment handler only upserts by file_id, so repeated polls for the same completed background task append duplicate download cards to the original message. Either avoid re-emitting attachments without file_id or make the live handler/upsert path dedupe by filepath too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6c6a34 — the live attachment upsert in useAttachmentHandler now keys by file_id ?? filepath, so re-emitted download-fallback attachments merge in place instead of appending a duplicate card per poll. Truly unkeyed types (web_search/file_search citations) keep the legacy append behavior.
| /** Ordering guard: a filename claim whose row was really written after | ||
| * this instant belongs to a NEWER run — the harvest must not overwrite | ||
| * it with stale bytes. */ | ||
| const freshClaimAfter = Date.now(); |
There was a problem hiding this comment.
For an older background code run that finishes after a later run has already written the same filename, this timestamp is after the later write, so the stale-harvest guard in processCodeOutput treats the existing claim as safe and overwrites the newer file contents. The comparison needs a time/order captured when the background task was dispatched (or otherwise tied to the original turn), not when the late harvest starts, or older background completions can corrupt newer code outputs in the same conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6c6a34 — the guard's anchor is now DISPATCH time: the handler threads the task's createdAt as dispatchedAt, and the harvest uses it (not harvest wall-clock) as freshClaimAfter. A slow old task settling after a newer run wrote the same filename now skips that file instead of overwriting it. Covered by a spec asserting the persister receives the dispatch timestamp and the passthrough into processCodeOutput.
| * `execute_code`/`bash_tool` are NOT excluded: they flow through the generic | ||
| * `ON_TOOL_EXECUTE` path, the detached invoke carries their code-session | ||
| * config, and their completion is harvested onto the dispatch turn's message | ||
| * (files persisted + tool-call output patched), with the exec session folded | ||
| * back into the run's shared code session on poll. |
There was a problem hiding this comment.
Making execute_code/bash_tool eligible here affects all agent runners, but I checked api/server/controllers/agents/openai.js and api/server/controllers/agents/responses.js: they pass backgroundToolNames/enable the capability while their toolExecuteOptions still omit persistBackgroundCodeResult and emitAttachment. On those OpenAI-compatible/Responses paths, a code tool can now advertise run_in_background, but the handler disables harvesting (persistBackgroundCodeResult == null), so generated files are only processed under the later poll tool call (or not anchored at all if the model never polls) instead of attaching to the original code run. Either wire the same persister/emitter for those routes or keep code tools excluded when harvest support is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6c6a34 — took the second option: the executor now downgrades code tool calls to FOREGROUND when the host wires no persistBackgroundCodeResult (same silent-downgrade pattern as ephemeral request-scoped MCP tools), so the OpenAI-compat/Responses routes can never strand generated files behind a poll. Wiring the full persister into those controllers stays a follow-up since their message-row semantics differ. Covered by a spec asserting a persister-less host runs the code call foreground with its real result and artifact.
| if (/^(Traceback|Execution error:|Error:|Exception:|.*Error:)/m.test(message)) { | ||
| return message; | ||
| } | ||
| return `Execution error:\n\n${message}`; |
There was a problem hiding this comment.
When a backgrounded code execution fails and the dispatch row is patched, this returns Execution error:\n\n..., but the code cards' failure detection (useToolCallState/isError and ERROR_PATTERNS in ExecuteCode) does not recognize the lowercase Execution error: prefix. Because the synthetic error status marker is only live SSE and is not persisted, reloading a failed background code run (or any path that only sees the patched output) renders it as a successful finished command with normal stdout styling. Use a prefix the client already treats as an error or extend the client matcher at the same time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6c6a34 — better than extending the matcher: patched failures now use the exact foreground failure wrapper the graph produces (Error: [toolName] tool call failed: <message>), which the client's isError detection and output styling already recognize. A reloaded failed background run renders as an error with the standard chrome, identical to a foreground failure.
…per Parity (Codex Round 5)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6c6a34f52
ℹ️ 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".
| } | ||
| try { | ||
| const Message = mongoose.models.Message as Model<IMessage>; | ||
| const result = await Message.updateOne({ messageId, user: userId, conversationId }, stages); |
There was a problem hiding this comment.
In resumable mode a client disconnect can persist an unfinished assistant row with the current content snapshot (api/server/controllers/agents/request.js saves unfinished: true partial messages). If a background code task settles while that partial row exists, this filter matches it and updateToolCallResult returns true, so the harvest stops retrying; when the request later saves the final response, it writes the in-memory content that still contains the background handle and overwrites the stdout/error patch for users who never poll. Avoid matching unfinished partial rows, or otherwise keep retrying until the finalized row/tool-call part is patched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 961e34a — updateToolCallResult now returns { matched, unfinished } (findOneAndUpdate with an unfinished projection), and the harvest loop treats a match on an unfinished partial row as non-terminal: the patch still lands (idempotent) but retries continue until a FINALIZED row holds it, so the finalize save's in-memory overwrite gets re-patched. Covered by a data-schemas spec on an unfinished: true row and a fake-timer harvest spec asserting the loop continues past the partial-row match.
| $in: [ | ||
| { $ifNull: ['$$existing.file_id', '$$existing.filepath'] }, | ||
| { $literal: attachmentKeys }, | ||
| ], |
There was a problem hiding this comment.
When two background code calls in the same assistant message generate the same filename, processCodeOutput reuses the existing file_id but returns attachments with different toolCallIds. This dedupe filter removes the earlier attachment solely because its file_id matches the later one, and the client maps attachments to cards by toolCallId, so the first code card loses its generated file. Include the tool-call identity in the idempotency key, or only replace attachments that match both the file key and the tool call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 961e34a — the attachment dedupe is now scoped to the tool call: an existing entry is replaced only when its key (file_id ?? filepath) matches AND its toolCallId equals the incoming call's, so sibling background calls sharing a claimed file_id keep their own card-anchored attachments while same-call re-application stays idempotent. Covered by a spec with two calls sharing a file_id on one message.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 961e34ad1a
ℹ️ 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".
| isUpdate && | ||
| freshClaimAfter != null && | ||
| claimed.updatedAt != null && | ||
| new Date(claimed.updatedAt).getTime() > freshClaimAfter |
There was a problem hiding this comment.
When two background code runs write the same filename, this timestamp guard can make the newer run drop its attachment if an older run writes after the newer run was dispatched but before the newer run harvests. In that sequence, the older run updates the shared claimed file row, claimed.updatedAt is now greater than the newer run's freshClaimAfter, and the newer output is skipped, leaving users with stale files from the earlier task. The guard needs an ownership/order signal that distinguishes which task wrote the row, not just whether any write happened after dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 664809c — the guard now compares WRITER dispatch order, not wall-clock write time: every code-output write stamps metadata.sourceDispatchedAt (harvests use their task's dispatch time; foreground writes use now ≈ their dispatch), and the skip condition reads the row's last-writer stamp (falling back to updatedAt for pre-stamp rows). An older task writing late no longer makes a newer task's harvest look stale — covered by a spec where the older writer's late write is overwritten by the newer task.
| const existingIndex = messageAttachments.findIndex( | ||
| (a) => (a as Partial<TFile>).file_id === fileId, | ||
| (a) => ((a as Partial<TFile>).file_id ?? (a as Partial<TFile>).filepath) === upsertKey, | ||
| ); |
There was a problem hiding this comment.
When two background code calls in the same assistant message generate the same filename, the server can legitimately send two attachment events with the same claimed file_id but different toolCallIds. This upsert key only compares file_id/filepath, so polling the second task replaces the first task's live attachment in messageAttachmentsMap, and the first code card loses its generated file until a reload repopulates from the DB. Include the tool-call id in the live attachment identity for tool-call-scoped artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 664809c — live attachment identity is now (file_id ?? filepath)::toolCallId in both the SSE upsert (useAttachmentHandler) and the DB↔live merge (useAttachments), so sibling code calls sharing a claimed file_id keep their own card-anchored attachments while same-call re-emits still merge in place. Covered by specs for the sibling-preservation case in both hooks.
| /** Error tasks carry their message in `error`, not | ||
| * `result`; both re-anchor the same patched part. */ | ||
| output: delivery.result ?? delivery.error, |
There was a problem hiding this comment.
For a background code task that is reaped by the running-task timeout, delivery.error is just Background task timed out; re-applying that raw string patches the original code card without the Error: [tool] tool call failed: prefix that the client uses to recognize failed code output on reload. Live polling briefly has the synthetic status: 'error' marker, but that marker is not persisted, so the reloaded conversation renders the timed-out code run as a normal completed output. Wrap code-task errors here the same way toCodeToolFailure() wraps thrown tool errors before anchoring them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 664809c — the poll-time re-anchor wraps error-status deliveries with toCodeToolFailure() (a no-op for already-wrapped detached failures), so a reaped task's raw Background task timed out patches the card in the client-recognized failure shape. Covered by a fake-timer spec that reaps a never-settling task past the running TTL and asserts the re-anchor output carries the wrapper.
…pper (Codex Round 7)
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db76e456f6
ℹ️ 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".
| const fileAttachments = attachments.filter((attachment) => { | ||
| const marker = attachment as { type?: string; status?: string } | undefined; | ||
| if (marker?.type !== BACKGROUND_STATUS_ATTACHMENT_TYPE) { | ||
| return true; | ||
| } | ||
| backgroundStatus = typeof marker.status === 'string' ? marker.status : 'completed'; |
There was a problem hiding this comment.
In an assistant message with multiple backgrounded code/bash calls, a completion marker for one call is stored in the message-level attachments array and then passed to every BashCall/ExecuteCode card. Because this helper treats any background_task_status marker as the current card's status without checking the marker's toolCallId, the first completed task makes sibling background cards show “finished” or “failed” and can also surface that task's files under the wrong card while their own handle is still running.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6095486, with one correction to the premise: attachments are not passed message-wide to every card — ContentParts routes them per-card by toolCallId (mapAttachments at client/src/components/Chat/Messages/Content/ContentParts.tsx builds partAttachments = attachmentMap[getToolCallId(part)]), and the marker always carries its originating toolCallId, so a sibling card cannot receive another call's marker or files through the normal path. That said, the scoping is now ALSO enforced in splitBackgroundAttachments as defense in depth (marker counted only when its toolCallId matches the card's, wildcard when either is absent), with a spec for the sibling-marker case.
| /** `timestamps: false`: a claim is an id reservation, not a content | ||
| * write — bumping `updatedAt` here would make the row look freshly | ||
| * written to the background harvest's out-of-order guard, which | ||
| * compares `updatedAt` against the harvest's start time. */ | ||
| { upsert: true, new: true, timestamps: false }, |
There was a problem hiding this comment.
With timestamps: false, a newly inserted code-file claim has neither updatedAt nor metadata.sourceDispatchedAt until the later content write. If a newer background task claims a filename first and an older task reaches the same claim while the newer write is still in flight, the older task sees no dispatch timestamp and the stale-output guard does not fire; if that older save finishes last, it overwrites the newer task's file despite being dispatched earlier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6095486 — claimCodeFile now stamps metadata.sourceDispatchedAt in $setOnInsert, so a freshly claimed (not-yet-written) row carries its claimant's dispatch order and an older task hitting the claim mid-flight trips the guard. Chasing this also surfaced a deeper bug your finding led me to: the File schema's metadata is a structured subdocument, so strict mode had been silently stripping the round-7 write stamp — added sourceDispatchedAt to the schema, with a real-Mongo spec proving insert-stamping and that a later claimant cannot overwrite the owner's stamp. A residual sliver remains when two writes are concurrently in flight after both guards passed (last-writer-wins, bounded to seconds, same property as concurrent foreground turns).
…rkers (Codex Round 8)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60954868d6
ℹ️ 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".
| const lastWriterDispatchedAt = | ||
| claimed.metadata?.sourceDispatchedAt ?? | ||
| (claimed.updatedAt != null ? new Date(claimed.updatedAt).getTime() : null); | ||
| if (isUpdate && freshClaimAfter != null && lastWriterDispatchedAt > freshClaimAfter) { |
There was a problem hiding this comment.
When an older background code task and a newer run generate the same filename concurrently, this guard can pass for the old task before the newer run writes its sourceDispatchedAt; the old task then continues through conversion/storage and later calls createFile(file, true) unconditionally, so it can overwrite the newer output after the newer write completes. Please make the stale-owner check part of the final file update/write (or re-check immediately before it) so overlapping same-filename harvests cannot corrupt the latest result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e143be0 — the stale-owner check is now part of the write: a new atomic confirmCodeFileOwnership (conditional findOneAndUpdate: succeed-and-stamp only when the row's sourceDispatchedAt is unset or not newer) runs immediately before every final createFile commit on shared rows, so an older harvest that passed the read guard cannot commit after a newer task claims ownership. Covered by a real-Mongo CAS spec and a process spec where ownership is lost between guard and commit. Remaining sliver: bytes already uploaded to the shared storage key by a loser whose upload interleaves — closing that needs per-file locking; unique output filenames sidestep it entirely.
| { | ||
| $and: [ | ||
| { $eq: ['$$part.type', 'tool_call'] }, | ||
| { $eq: ['$$part.tool_call.id', toolCallId] }, |
There was a problem hiding this comment.
In multi-agent runs, provider tool-call IDs can repeat across agents in the same response, but this patch matches only tool_call.id. If two agents both emit an ID like call_0, one background code harvest will update every matching content part and its toolCallId attachments will be routed to the sibling card too. Include the producing agent/part identity in the persisted content and update filter, or otherwise make the IDs unique before saving.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e143be0 — the registry now stores the dispatching agentId (it was already part of the task dedupe key for exactly this reason), threads it through harvest and re-anchor, and updateToolCallResult scopes the part match: tool_call.id must match AND the part's agentId (part-level or tool_call-level, both are in PartMetadata) must be absent or equal. Covered by a real-Mongo spec with two agents' call_0 parts in one message where only the producing agent's part is patched.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e143be0661
ℹ️ 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 (isUpdate && freshClaimAfter != null) { | ||
| const stillOwner = await confirmCodeFileOwnership({ file_id, sourceDispatchedAt }); |
There was a problem hiding this comment.
When two background code tasks in the same conversation produce the same filename, the older task can be the one that inserts the claim (isUpdate === false) and then continue downloading/converting while a newer task reclaims/stamps and writes the same file_id. Because this CAS is skipped for the initial claimant, the older task can later createFile over the newer task's bytes/metadata, defeating the stale-output guard for exactly the overlapping background case it is meant to protect; the ownership check needs to run for background writes even when this call inserted the row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7d0180 — the pre-write CAS now runs for ALL background harvests, including the claim's inserter (isUpdate removed from the gate). An older inserter overtaken mid-download by a newer task's reclaim-and-write fails confirmCodeFileOwnership at commit time and skips. Covered by a spec where the insert-path claimant loses ownership before the commit.
| if (result?.file) { | ||
| attachments.push(result.file); |
There was a problem hiding this comment.
In a handoff response where two agents both get provider id call_0, the output patch is scoped with agentId, but the harvested files stored here only carry toolCallId. The client routes attachments by toolCallId alone (mapAttachments), so files from agent B's background code call will be delivered to every call_0 card in that message, including agent A's sibling call; include and route an agent discriminator with these harvested/status attachments or they can appear on the wrong tool call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7d0180 — harvested attachments and the status marker now carry the dispatching agentId (threaded through processCodeOutput), and the client's per-part lookup applies filterAttachmentsForPart: an attachment with an agentId only routes to the part whose agent matches (part-level or tool_call-level identity), with missing ids as wildcards so single-agent runs and legacy rows behave exactly as before. Covered by unit specs for the filter and pass-through assertions in the harvest specs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7d018066f
ℹ️ 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".
| try { | ||
| emitAttachment({ | ||
| type: BACKGROUND_STATUS_ATTACHMENT_TYPE, | ||
| file_id: `bg-${delivery.toolCallId}`, |
There was a problem hiding this comment.
In handoff responses where multiple agents each emit a background code call with the provider-reused id call_0, this synthetic marker gets the same file_id (bg-call_0) for every agent even though agentId is present. The SSE attachment handler upserts by file_id/toolCallId, so the second agent's poll overwrites the first agent's marker; stdout-only background runs rely on this marker to leave the “Running in background” state, so one card can remain stuck until reload. Include the agent id in the marker's stable key, or make the client merge key include agentId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest commit — the marker's stable id is now agent-suffixed (bg-<toolCallId>-<agentId>), so sibling agents' call_0 markers upsert independently and each card's stdout-only completion signal survives. The card-side routing filter already scoped markers by agent (round 10); this closes the upsert-key half.
| { $literal: attachmentKeys }, | ||
| ], | ||
| }, | ||
| { $eq: ['$$existing.toolCallId', toolCallId] }, |
There was a problem hiding this comment.
This dedupe removes existing attachments solely by file_id/filepath and toolCallId, but the new background code path explicitly handles handoff messages where provider tool-call ids repeat across agents. If two agents with the same provider id generate the same filename in one conversation, claimCodeFile gives them the same file_id; when the second task is anchored, this filter drops the first agent's attachment even though its card is distinct and agentId is carried on the attachment. Scope the replacement condition by agentId as well, treating missing ids as the legacy wildcard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest commit — the anchor-time dedupe now also requires an agent match (existing attachment's agentId absent or equal, missing ids = legacy wildcard), so a sibling agent's attachment under the same provider id + claimed file_id survives the second agent's patch. Covered by a real-Mongo spec with both id and file key colliding across agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/danny-avila/LibreChat/blob/1269952ed00bbf8ef8aa0a3448e7858132ad7889/client/src/components/Chat/Messages/Content/ContentParts.tsx#L325-L327
For grouped tool-call rendering, this still aggregates attachments by toolCallId alone and bypasses the new agentId filter used by individual parts. In a handoff response where two adjacent agents both emit call_0, the group receives the same mixed attachment list once per part (e.g. [a,b,a,b]), so generated files can render duplicated or under the wrong grouped call even though the single-part path is scoped correctly. Please apply filterAttachmentsForPart with each part's agent id before flattening the group attachments.
ℹ️ 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".
| return false; | ||
| } | ||
| } | ||
| await createFile(fileData, true); |
There was a problem hiding this comment.
When two background code runs emit the same filename, this CAS is not actually protecting the final write: confirmCodeFileOwnership and createFile are separate awaited operations, so an older task can pass the check, a newer task can then stamp/write the row, and the older task's unguarded createFile can still overwrite the newer file metadata (and point subsequent sandbox priming at stale output). Please fold the ownership predicate into the final update/write or otherwise hold the claim through the write so stale harvests cannot win this race.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4a5a2a — the claim-confirm two-step is gone. commitCodeFile now performs a single atomic conditional updateFile: the ownership predicate (metadata.sourceDispatchedAt absent, or <= this write's dispatch order) lives in the update filter itself, so the check and the write are one MongoDB operation and a newer writer landing between them can no longer be clobbered. confirmCodeFileOwnership is removed entirely (method, interface, and registration), and process.spec covers both the accepted and rejected (null return → skip) paths of the conditional commit.
| conversationId, | ||
| messageId, | ||
| session_id, | ||
| agentId, |
There was a problem hiding this comment.
In handoff/multi-agent runs where providers reuse tool-call ids, this new agentId is what keeps a background code artifact scoped to the originating card. The normal success paths add it to returned file metadata, but the download-fallback paths still return attachments without agentId, so oversized files or storage/download failures become wildcard attachments that can be shown on, or deduped against, sibling agent calls with the same toolCallId. Please thread agentId through createDownloadFallback as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4a5a2a — createDownloadFallback now takes agentId and stamps it on the fallback record, and all three call sites pass the emitting agent's id through, so fallback download rows carry the same agent scoping as primary file rows and the agent-scoped dedupe/routing treats them consistently.
| } | ||
| const dbToolCallId = toolCallIdOf(db); | ||
| const liveToolCallId = toolCallIdOf(live); | ||
| return dbToolCallId == null || liveToolCallId == null || dbToolCallId === liveToolCallId; |
There was a problem hiding this comment.
When the same claimed file_id is attached to multiple tool calls, this stricter match requires the live preview overlay to carry a compatible toolCallId. However useAttachmentPreviewSync still upserts terminal preview records by file_id only and merges into the first existing live entry, so only one sibling attachment gets the ready/failed overlay while the others remain pending on loaded conversations even though the file preview finished. Please update preview sync to match/upsert by file_id plus compatible toolCallId, or fan out bare preview updates to all matching attachments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4a5a2a — useAttachmentPreviewSync now fans the terminal preview record out to every live entry sharing the file_id (the preview is a per-file result, so ready/failed + text/textFormat/previewError apply to all siblings regardless of toolCallId). The insert path for loaded conversations is unchanged. Added a spec seeding two sibling entries (same file_id, different toolCallIds) plus an unrelated file and asserting both siblings resolve while the third stays pending.
| if (task.artifact == null && artifact != null) { | ||
| task.artifact = artifact; | ||
| task.artifactDelivered = false; |
There was a problem hiding this comment.
If a code artifact exceeds MAX_ARTIFACT_CHARS, complete() intentionally drops it via toStoredArtifact, but when the detached harvest later fails this recovery path restores the original unbounded artifact from the harvest closure. In a failing persister/storage scenario, a large code artifact that was supposed to be discarded can be retained in the registry until TTL, defeating the memory cap for background tasks. Please run restored artifacts through the same toStoredArtifact guard before assigning them back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4a5a2a — restoreArtifact now routes the incoming artifact through toStoredArtifact(taskId, artifact) before storing, so the same size bound applied at completion time also applies on poll-time re-anchor and an oversized restored artifact can no longer bypass the cap.
- Replace the two-step claim-confirm CAS with an atomic conditional updateFile: the ownership predicate (no sourceDispatchedAt, or <= this write's dispatch order) moves into the update filter, removing confirmCodeFileOwnership and the lost-update window between check and write - Thread agentId through createDownloadFallback so fallback download rows scope to the emitting agent like primary rows - Fan terminal preview overlays out to every live attachment sharing the file_id in useAttachmentPreviewSync (sibling tool calls no longer stick on pending) - Restore background artifacts through toStoredArtifact so the size bound applies on re-anchor - Apply filterAttachmentsForPart to grouped tool-call attachments in ContentParts so handoff agents with colliding provider call ids do not cross-contaminate groups
|
@codex review Round 12 fixes are in d4a5a2a, including the review-body finding (grouped attachments filter): ContentParts' grouped rendering now applies filterAttachmentsForPart with each part's agent id before flattening, so handoff agents sharing a provider call id no longer duplicate or cross-route generated files in grouped tool calls. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4a5a2abcc
ℹ️ 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".
| const existingToolCallId = (a as { toolCallId?: string }).toolCallId; | ||
| return ( | ||
| existingToolCallId == null || | ||
| incomingToolCallId == null || | ||
| existingToolCallId === incomingToolCallId |
There was a problem hiding this comment.
When two handoff agents emit attachments for the same generated filename, processCodeOutput can give both entries the same file_id, and provider tool ids like call_0 can also repeat across agents. This live SSE upsert only compares the file key and toolCallId, so the second agent's event merges over the first and overwrites its agentId; after that ContentParts has only one attachment to filter, causing one sibling card to lose or receive the wrong generated file until the DB snapshot is reloaded. The same match needs to treat distinct non-null agentIds as separate entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a5a1510 — the live SSE upsert now applies the same wildcard semantics to agentId as it does to toolCallId: a missing agentId on either side merges (bare deferred-preview updates, single-agent runs), but distinct non-null agentIds stay separate entries. The useAttachments DB/live merge needed the same treatment or it would have re-merged what the SSE handler kept apart: attachmentKey now includes agentId (fileKey::toolCallId::agentId), matchesLiveEntry rejects distinct agents, and less-specific key variants are registered so agent-less overlaid records still dedupe. Specs cover colliding file_id + call_0 across two agents in both hooks (entries stay separate, bare updates still merge, sibling live entry survives the DB merge).
| const lastWriterDispatchedAt = | ||
| claimed.metadata?.sourceDispatchedAt ?? | ||
| (claimed.updatedAt != null ? new Date(claimed.updatedAt).getTime() : null); | ||
| if (isUpdate && freshClaimAfter != null && lastWriterDispatchedAt > freshClaimAfter) { |
There was a problem hiding this comment.
When two background code calls are dispatched in the same millisecond and both write the same filename, their sourceDispatchedAt values can tie because the stamp ultimately comes from Date.now(). This stale guard only rejects strictly newer stamps, so an older task with an equal stamp can still commit after the newer one and overwrite the file record/attachment the guard is meant to protect; use a per-dispatch monotonic sequence or tie-breaker rather than millisecond wall time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a5a1510 — background task createdAt (the source of sourceDispatchedAt) now comes from a strictly-increasing per-process dispatch counter: nextDispatchStamp returns Date.now() when it advances and lastStamp+1 on a same-millisecond tie, so two distinct dispatches can never carry equal stamps while equal-stamp acceptance ($lte) is preserved for idempotent re-commits of the same task. Process-local is sound here because the background registry itself is per-process. Added a spec freezing Date.now and asserting two same-millisecond dispatches get strictly increasing createdAt.
… Round 13) - Scope the SSE attachment upsert and the useAttachments DB/live merge by agentId with the same wildcard semantics as toolCallId: distinct non-null agentIds stay separate entries, so handoff agents sharing a claimed file_id and a repeated provider tool id (call_0) no longer merge over each other's cards - Extend the attachment identity key to fileKey::toolCallId::agentId and register less-specific key variants so bare and agent-less live records still dedupe after overlay - Stamp background task createdAt from a strictly-increasing per-process dispatch counter: raw Date.now() can tie for same-millisecond dispatches and the stale-output guard accepts equal stamps (needed for idempotent re-commits), which would let an older task overwrite a newer task's committed file
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
Summary
I made
execute_codeandbash_toolnatively backgroundable on the background tool-call substrate from #14197, so the model can dispatch long-running code, keep the conversation moving, and collect stdout, generated files, and sandbox session state later — with results anchored to the original tool call instead of the poll turn. No@librechat/agentschanges were required: code tools already flow through the hostON_TOOL_EXECUTEexecutor, and the prior exclusion was host policy only.execute_code/bash_toolfromEXCLUDED_BACKGROUND_TOOL_NAMES; opt-in rides the sametool_options[name].run_in_backgroundcontract as MCP tools, and the ephemeral/model-spec blanket toggles now cover code tools.applyBackgroundToolCalls: the runtime definition isbash_tool(execute_codeis the capability marker that expands at load time), so an opt-in keyed by either name enables both.buildToolCallConfighelper —session_id,_injected_files, and_runtime_session_hintnow carry over, where the previous detached path would have run fileless on the Code API's default runtime session.complete()on the row patch would livelock same-turn polls onrunning, since the dispatch turn's message row only exists after that turn finalizes.createBackgroundCodeResultHandler) that persists generated files with the originalmessageId/toolCallId, patches the dispatch turn's tool-calloutputfrom the handle JSON to real stdout, and appends attachments to the original message row — so a backgrounded run reads like a foreground one on reload and in later model turns, and next-turn file priming picks up outputs even if the model never polls.updateToolCallResultto data-schemas as an idempotent aggregation-pipeline update (content-part patch + attachment dedupe byfile_id), retried on a backoff schedule while the dispatch row does not exist yet and safely re-applied later.check_background_taskas a code-session participant and returned the claimed artifact on the poll result, so the SDK folds the exec session/files back intoGraph.sessionsand same-run foreground code calls see the background run's outputs.toolEndCallbackdelivery MCP tools use today.useAttachmentsto keep live-only SSE entries instead of treating the DB list as exhaustive — attachments are only emitted after persistence, so dropping them hid harvested files until a full reload.ExecuteCode/BashCallvia a sharedparseBackgroundHandlehelper ("Running in background" / "Finished in background") instead of showing the dispatch handle JSON as stdout.run_in_backgroundcapability, writingtool_optionsfor both code tools.Known limits: the Code API
/exectransport stays synchronous, so maximum runtime remains its server-side cap; the row patch gives up if the dispatch turn outlives the ~16-minute retry schedule (poll delivery still works); and a full-row save landing after the last poll is not healed.Change Type
Testing
handlers.background.spec.tswith a backgrounded-code suite: detached invoke receives the full session config, completion is not gated on the harvest (same-turn polls seecompletedwhile the persister blocks), error paths patch the dispatch turn, harvested attachments re-emit on poll with the session-fold artifact riding the poll result, and hosts without a persister fall back to poll-turn delivery.background.spec.tsfor the new registry surface (harvestStarted,attachHarvest,getBackgroundCodeDelivery, claim/restore round-trips) and theexecute_code→bash_toolopt-in expansion.updateToolCallResultcoverage inmessage.spec.ts(mongodb-memory-server): targeted part patch, atomic attachment append, idempotent re-apply without duplicates, cross-user isolation, and row-missing retry signaling.callbacks.background.spec.jsfor the harvest handler: original-identity file processing, inherited-file skips, deferred preview finalization, retry backoff with fake timers, reapply mode, and best-effort file failures.parseBackgroundHandleand theBashCallbackground states; updated theuseAttachmentsmerge contract test.packages/apiagents (1,349),data-schemas(1,835 + 5 new), client Parts/hooks sweeps, lint and typecheck clean on all touched files.bash_toolopt-in gap.Test Configuration:
Checklist