Repository navigation
🚕 feat: Add Per-Call Subagent Machine Selection - #16757
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0b926923e5
ℹ️ 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".
| * Host argument declaration accepted by `@librechat/agents` subagent configs | ||
| * (`SubagentConfig.hostArgs`, added after 4.0.2). Declared locally so this | ||
| * package typechecks against older SDKs, which ignore the field. |
There was a problem hiding this comment.
The checked-in lockfile still installs @librechat/agents 4.0.1 (package-lock.json lines 9219-9222), while this code states that hostArgs was added after 4.0.2. Consequently an npm ci deployment has no SubagentHostArgumentError, isSubagentHostArgsSupported() is always false, and describe() never advertises the machine or workspace arguments, leaving the feature entirely inactive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional for now, and the PR stays a draft until it is resolved. The SDK side is LibreChat-AI/agents#596, which is not released yet, so there is no version to bump to. Until then the declarations are gated on the installed SDK exporting SubagentHostArgumentError, which keeps this change inert (no schema change, unchanged behavior) on 4.0.x rather than breaking it. The merge order in the description is: release agents#596, merge #16756, then bump @librechat/agents here, drop the local types and the test stand-in, and mark ready.
| toolNames: string[], | ||
| agentId?: string, | ||
| signal?: AbortSignal, | ||
| executionContext?: SubagentExecutionContext, |
There was a problem hiding this comment.
When run-file sharing is inactive, createChatRunFileBindings.wrapProvision calls its fallback with only (names, agentId, signal) and drops the fourth executionContext argument. For repeated or concurrent calls of the same subagent routed to different machines, this makes resolveExecutionContext miss and provisioning falls back to the single per-agent context, so attachments are uploaded to one sibling's machine while the selected sibling executes on another machine.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 034ee0f: wrapProvision now forwards the execution context to the fallback when run-file sharing is inactive, so resolveExecutionContext sees the routed child's execution.
| for (const agent of agents) { | ||
| const context = agent?.codeExecutionContext; | ||
| if (context?.statefulSessions !== true) { | ||
| continue; | ||
| const agentId = agent?.id ?? null; | ||
| const contexts = [agent?.codeExecutionContext, ...(agent?.codeExecutionChoices ?? [])]; |
There was a problem hiding this comment.
Adding every codeExecutionChoices entry can produce more than 128 targets—for example, five reachable subagents each allowed on the valid maximum of 32 conversation machines. captureCodeExecutionApprovalBinding() persists all of them, but isCodeExecutionApprovalBinding() rejects any binding over MAX_CODE_EXECUTION_APPROVAL_TARGETS, so a stateful-code approval created for such a valid graph can never resume and always reports that the execution target changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 034ee0f: each agent's per-call choices fold into a single approval target (a hash over the sorted choice hashes), so an agent contributes at most two targets regardless of how many machines it may use. A test binds five agents with 32 choices each into five targets and re-asserts it.
| if (!isSubagentHostArgsSupported() || flags.statefulCodeSessions !== true) { | ||
| return {}; | ||
| } | ||
| const { targets } = await resolveSubagentCodeTargets(paramsFor(agent, flags)); |
There was a problem hiding this comment.
Each unique lazy subagent calls resolveSubagentCodeTargets() independently during startup, and that function performs a live worker-status request for every admitted machine without request-scoped reuse. A valid graph of 50 subagents and 32 selections therefore performs up to 1,600 largely duplicate bridge requests before initialization completes, adding substantial latency and load; cache each worker capability result for the request while still deriving agent-specific contexts separately.
AGENTS.md reference: AGENTS.md:L186-L188
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The live checks go through the module-level createCodeBridgeStatusPoller, which coalesces in-flight requests per worker and caches each result for 2 s (and remembers a ready lease). Descriptors are built concurrently (Promise.all over children), so N children over the same M machines issue about M bridge requests, not N x M; agent-specific contexts are still derived per child. A second request-scoped cache would duplicate that layer.
| /** Assigned rather than spread so the field typechecks against SDKs that predate it. */ | ||
| return child.subagentHostArgs == null | ||
| ? config | ||
| : Object.assign(config, { hostArgs: child.subagentHostArgs }); |
There was a problem hiding this comment.
hostArgs is attached only by createLazySubagentConfig, but buildLazySubagentDescriptors returns an already initialized config whenever the same child is also a handoff/parallel agent. That config takes the eager subagentAgentConfigs branch in buildSubagentConfigs, which never adds host arguments or a per-call resolver, so this valid overlap configuration silently loses the machine and workspace choices even after the SDK dependency is upgraded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as a documented limitation. A child that is also a root (handoff/parallel) agent is initialized eagerly for that role before any subagent call exists, and the SDK only accepts host arguments on lazy configs (it rejects them on eager ones), because only a resolver can act on a per-call value. Routing such an overlap per call would mean re-initializing an agent the run already owns, which is out of scope here; that child keeps its existing route.
| context.signal, | ||
| ); | ||
| throwIfAborted(context.signal); | ||
| const routedAgent = placement.agent; |
There was a problem hiding this comment.
When a parent routes a child to a non-default machine, only the child's copied agent document and per-execution tool context are updated. The request-wide req.codeWorkspaceInheritance map was computed earlier from saved/default routes and is not updated per execution, so if this routed child delegates to a grandchild without an explicit machine, the grandchild inherits the original machine rather than the machine its actual parent is running on, potentially reading or modifying the wrong workspace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 034ee0f: a routed child's execution records the machine it was routed to, and its own children (whose resolver context carries the parent execution as parentRunId) inherit that machine when they may reach it, ahead of the request-wide map. An explicit choice on the grandchild still wins, and a grandchild that cannot reach the parent's machine keeps its own authorized route. Covered by a test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 034ee0f77a
ℹ️ 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 resolution = await resolveSubagentCodeTargets(paramsFor(agent, flags)); | ||
| const target = requested | ||
| ? selectSubagentCodeTarget(hostArgs, resolution) | ||
| : resolution.targets.find((candidate) => candidate.environmentId === parentEnvironmentId); | ||
| return target == null | ||
| ? { agent } | ||
| : { agent: placeSubagentOnCodeTarget(agent, target), target }; |
There was a problem hiding this comment.
When a dynamically routed parent spawns an agent without stateful code, resolveSubagentCodeTargets() returns no targets, so this returns the child unchanged and never records parentEnvironmentId. If that child then spawns a code-enabled grandchild, environmentFor(context.parentRunId) cannot recover the dynamic route and the grandchild falls back to the statically computed/default machine; this breaks the existing resolveCodeWorkspaceInheritance() behavior that non-code agents pass their parent's machine through and can send the grandchild to the wrong workspace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 092a13c: an agent that runs no stateful code now records its routed parent's machine for its own execution, so a code-enabled grandchild reached through it inherits that machine (when it may reach it), matching #16756's pass-through rule. Covered by a parent -> non-code writer -> tester test.
| if (metadata.codeWorkspaceUnavailable && hostArgs == null) { | ||
| throw new CodeWorkspaceSelectionError(metadata.codeWorkspaceUnavailable); |
There was a problem hiding this comment.
When the child's saved/default worker is unavailable but a dynamically routed parent is running on another allowed, reachable machine, metadata records codeWorkspaceUnavailable before the per-call parent route is known. An omitted machine is precisely how the child requests inheritance, but this guard throws before subagentCodeRouting.place() can select the parent's healthy target, so nested inheritance fails unless the model redundantly supplies an explicit host argument. Keep this availability decision in the TypeScript routing boundary and evaluate the inherited route before rejecting the call.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 092a13c: the unavailable-default decision moved into the routing session. place first tries an explicit call choice, then the machine a routed parent runs on, and only refuses with the default route's CodeWorkspaceSelectionError when neither applies; the descriptor no longer throws before routing. Covered by a test.
| const placement = await waitForAbort( | ||
| subagentCodeRouting.place( | ||
| agent, | ||
| codeFlags ?? {}, | ||
| getSubagentHostArgValues(context), | ||
| subagentPlacements.environmentFor(context.parentRunId), |
There was a problem hiding this comment.
This change adds route selection, host-argument interpretation, placement validation, and execution-scoped routing state directly to the legacy /api initializer rather than leaving it as wiring. Extract the orchestration into a dependency-injected TypeScript service under packages/api and have this file call that boundary; otherwise this substantial backend behavior remains outside the repository's required module boundary.
AGENTS.md reference: AGENTS.md:L73-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 092a13c: createSubagentCodeRouting in packages/api/src/code/targets.ts is now a request-scoped session that owns host-argument interpretation, parent-route inheritance, the unavailable-default decision, placement validation, per-execution tool contexts and the routed-config check. initialize.js only constructs it and passes values through (describe, place, attach, getToolContext, isRouted).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 092a13c176
ℹ️ 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 (requested) { | ||
| const resolution = await resolveSubagentCodeTargets(paramsFor(agent, flags)); | ||
| const target = selectSubagentCodeTarget(hostArgs, resolution); | ||
| if (target != null) { | ||
| return routeTo(agent, target); |
There was a problem hiding this comment.
When a lazy child's default workspace is unavailable but another advertised machine is healthy, this branch allows the child to initialize on that attached machine even though its descriptor has no codeExecutionContext. If no root or other descriptor initially targets an attached environment, createRun sees an empty attachedCodeEnvironmentAgentIds set, skips both assertAttachedCodeEnvironmentApprovalSupported and creation of createAttachedCodeEnvironmentPolicyHook (packages/api/src/agents/run.ts), and the later resolved-agent callback can only mutate sets that no installed hook reads. Thus this fallback path can execute against an attached machine without the required machine permission/approval policy; alternate attached targets must cause that policy hook and HITL capability check to be installed before the run starts.
AGENTS.md reference: AGENTS.md:L186-L189
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd35232: collectAttachedCodeEnvironmentAgentIds now counts a descriptor whose per-call choices include an attached machine, so the attached-machine policy hook and the HITL capability check are installed before the run starts even when no default route is attached. The resolved agent's machine settings are registered on resolution as before. Covered by a test.
| if (executionId && placement.childEnvironmentId != null) { | ||
| childRoutes.set(executionId, placement.childEnvironmentId); | ||
| } | ||
| if (placement.target == null || !executionId) { | ||
| contexts.set(agentId, toolContext); | ||
| return; | ||
| } | ||
| routedContexts.set(executionId, { agentId, toolContext }); |
There was a problem hiding this comment.
When concurrent calls invoke the same child on machines with different permission settings, tool contexts are separated by execution here, but createRun still stores attachedCodeEnvironmentSettings only by agent ID. Each lazy resolution overwrites that single entry in registerResolvedMCPToolAliases, while createAttachedCodeEnvironmentPolicyHook looks it up using only executingAgentId, so the last-resolved sibling's policy governs both executions; for example, machine B's allow can replace machine A's deny or ask. Preserve the selected policy per execution and consult the trusted execution context during PreToolUse.
AGENTS.md reference: AGENTS.md:L186-L189
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd35232 by making the per-agent policy correct rather than re-keying it: the routing session now keeps one machine per subagent for the whole request (the same "one route per subagent" rule #16756's inheritance uses). A later call that omits the machine follows the subagent's earlier per-call route, and a call that would move it to another machine is refused with a model-visible machine error, so two executions of one agent can never run on machines with different permission settings and the agent-keyed policy entry always matches. Covered by a test.
| subagentHostArgs, | ||
| codeExecutionChoices, |
There was a problem hiding this comment.
When a lazy child is routed to a non-default machine and the conversation history references a file-backed skill, this new choice is not included by collectCodeExecutionProfileRoutes, which traverses codeExecutionContext but ignores codeExecutionChoices. Consequently primeInvokedSkillsForProfiles uploads and seeds the skill files only for the descriptor's default route (or for no route when that default is unavailable), and the selected alternate route's codeSessionKey starts without the historical skill files. Include every choice in profile collection or prime the selected profile during lazy resolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd35232: collectCodeExecutionProfileRoutes now adds every codeExecutionChoices route (with that route's session key), so historical skill files are primed for each machine a child may be routed to. Covered by a test.
| subagentHostArgs, | ||
| codeExecutionChoices, | ||
| subagentCodeFlags: { statefulCodeSessions, statefulCodeEnvironment }, |
There was a problem hiding this comment.
When the only code-capable agent is a lazy child whose default workspace is unavailable but an alternate machine is healthy, the descriptor keeps codeEnvAvailable false while these raw flags still let place() initialize it on the alternate route. anyAgentHasCodeEnv therefore returns false before the SDK run is compiled and leaves toolOutputReferences disabled; once resolved, the child can register the bash tool, but its {{tool<idx>turn<turn>}} references pass to the shell unsubstituted, exactly the failure the gate in packages/api/src/agents/run.ts is intended to prevent. Treat a reachable code choice as code availability for this run-wide gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd35232: the run-wide tool-output-reference gate treats a descriptor with per-call choices as code-capable, so a child that can only run code through a choice still gets substitution.
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
A parent agent can now pass optional `machine` / `workspace` arguments on a subagent call to pick where the child runs, using the host-argument extension point added to @librechat/agents. - Each lazy child declares enums built only from machines it may reach: admitted in the conversation's sealed codeWorkspaces, on its default or allowlist, in the requesting principal's tenant-scoped list, and served by a ready worker. A child the conversation pinned to a machine offers only that machine; a child with no reachable machine declares nothing. - On use, a request-scoped routing session in packages/api re-validates the choice against the current request and live worker status and refuses an invalid one with a typed, model-visible error instead of falling back. A routed child is pinned to the chosen machine at every routing site, overriding parent inheritance. - Omitted arguments keep inherit-by-default routing, extended so a child follows the machine a per-call-routed parent runs on (passing through agents without code). A subagent keeps one machine for the whole request. - A child whose default workspace is unavailable stays guarded unless it has per-call choices; then it stays callable and its description asks the parent to pick a listed machine. - Routed children keep their tool context per execution for tool loading, provisioning and artifact handling. - Paused-approval bindings, the attached-machine policy hook, skill-file priming and tool-output references account for per-call choices. The declarations are gated on the installed SDK exporting SubagentHostArgumentError, so this change is inert until LibreChat depends on an @librechat/agents release that includes it.
bd35232 to
45ad7a6
Compare
lia-by-librechat
Bot
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45ad7a6ebb
ℹ️ 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 (unavailableReason != null) { | ||
| throw new CodeWorkspaceSelectionError(unavailableReason); | ||
| } | ||
| return { agent }; |
There was a problem hiding this comment.
When two parallel calls target the same lazy child and one omits machine while the other requests an alternate, this default branch returns without reserving the child's default in routeByAgent; that reservation occurs only in attach() after the asynchronous initializeAgent call. The alternate call can therefore claim another machine while the default call is initializing, allowing both executions to resolve on different machines, after which registerResolvedMCPToolAliases overwrites the attached-machine permission settings keyed only by agent ID and one execution may receive the other's allow/deny/ask policy. Fresh evidence on this head is that the new one-machine enforcement still leaves the default route unclaimed across that initialization window; reserve it synchronously in place() or key the policy by execution.
AGENTS.md reference: AGENTS.md:L186-L189
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d2a990: place now reserves the default route synchronously, computing it with the same resolveCodeExecutionContext inputs initializeAgent uses (including #16756's request-scoped inheritance via getInheritedEnvironments), so a concurrent call naming another machine is refused while the default call initializes; naming the same machine still works. Covered by tests for a plain default and an inherited default.
| if (!subagentCodeRouting.isRouted(context.executionId)) { | ||
| graphMemberConfigsById.set(config.id, config); | ||
| } | ||
| await resolveGraphSubagentsFor(config, context.signal); |
There was a problem hiding this comment.
When a lazy child is explicitly placed on a non-default machine and then invokes one of its subagents.graphs, the per-call placement stops at the child: resolveGraphSubagentsFor initializes each member through loadGraphMember with only { signal }, so no parentRunId, codeFlags, or routed environment reaches subagentCodeRouting.place. The member consequently uses req.codeWorkspaceInheritance, which was computed from the parent's saved/default route, even though static inheritance explicitly treats graph members as children; it can therefore read or modify a different workspace from the routed parent. Fresh evidence on this head is that routed configs avoid the global cache here, but their graph-member initialization still never consumes placement.childEnvironmentId; propagate the routed execution context when resolving the graph.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d2a990: when a per-call-routed subagent resolves its subagents.graphs, members are initialized with the routed execution as their parent (loadRoutedGraphMember), so each follows the routed machine when it may reach it, with its own code flags; such routed members are not put in the shared graph-member cache. A member that already initialized on its own route this request keeps it (one machine per agent per request). Covered by an initialize test where a routed reviewer's graph member lands on the reviewer's machine.
…ph Members - Reserve a subagent's default machine when it is placed, as initializeAgent will resolve it (including parent inheritance), so a concurrent call cannot claim another machine during initialization. - Graph members spawned by a per-call-routed subagent follow its machine when they may, unless they already initialized on their own route this request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d2a990190
ℹ️ 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 { targets } = await resolveSubagentCodeTargets(paramsFor(agent, flags)); | ||
| const target = targets.find((candidate) => candidate.environmentId === inherited); | ||
| if (target != null) { | ||
| return routeTo(agent, target); |
There was a problem hiding this comment.
The new default-route reservation does not cover two concurrent omitted-machine calls that reach the same child from parents routed to different machines: both can compute inherited before either route is claimed, await target resolution, and then this branch commits its target without another conflict check. Both executions can consequently initialize on different machines, violating the one-machine-per-agent invariant; because registerResolvedMCPToolAliases stores attached-machine permission settings by agent ID, the later resolution can make one execution use the other machine's allow/deny/ask policy. Recheck or atomically reserve the inherited target after the await.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aaeef87. place now re-reads the inherited route after the resolveSubagentCodeTargets await and only then commits it, so if a concurrent call settled this subagent meanwhile, the later call follows that machine (or falls back to the reserved default) instead of claiming its own parent's. Covered by a test where two omitted-machine calls from parents routed to different machines settle on one machine.
| if (conflicts(agent.id, target.environmentId)) { | ||
| throw createSubagentHostArgumentError(SUBAGENT_MACHINE_ARG, 'unavailable'); |
There was a problem hiding this comment.
When an earlier call has claimed one machine and a later call supplies only the workspace for another machine, target selection succeeds and this conflict branch always rejects machine. Because workspace is explicitly advertised as an alternative argument and the SDK exposes argument-specific recovery guidance, the parent is told to change or omit an argument it never supplied and may repeatedly retry the same invalid workspace choice. Reject workspace when machine is absent, while retaining machine for machine-based or combined selections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aaeef87: a conflict is attributed to workspace when the call supplied only workspace, and to machine for machine-based or combined selections. Covered by a test.
| if (!subagentCodeRouting.isRouted(executionId)) { | ||
| graphMemberConfigsById.set(memberId, config); | ||
| } | ||
| return config; |
There was a problem hiding this comment.
When a routed lazy child sends a graph member to an alternate machine, this newly initialized member is returned without being passed through the resolved-agent registration callback; createLazySubagentConfig only registers the outer child. The run-wide attachedCodeEnvironmentSettings entry for the member therefore remains the policy captured from its lightweight metadata and default machine, so PreToolUse can apply that machine's allow decision while the member is actually executing on an alternate machine configured as deny or ask. Register each dynamically initialized routed graph member's selected configuration before exposing its graph inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b55612a / c78629f: the lazy resolver now passes each graph member the resolved child initialized through the resolved-agent callback before building the child's inputs, so the run-wide attached-machine policy entry for a routed member is the one of the machine it actually runs on. The test drives createRun's real PreToolUse hook: the member flips from its metadata machine's allow to its routed machine's deny once the parent resolves.
…embers - Re-read a subagent's inherited route after resolving targets, so two concurrent calls from differently routed parents settle on one machine. - Attribute a workspace-only conflict to the workspace argument. - Register graph members a routed subagent initialized, so each gets the attached-machine policy of the machine it actually runs on.
- Re-read a subagent's inherited route after the target resolution await, so two concurrent omitted-machine calls from differently routed parents settle on one machine. - Attribute a workspace-only conflict to the workspace argument.
…h createRun - Register a resolved child's graph members inline in the lazy resolver instead of through an exported generic helper whose test did not typecheck. - Drive the check through createRun's real PreToolUse hook: a member routed to a deny machine flips from its metadata machine's allow to deny once its parent resolves.
…nitializes - Pass a graph member's code flags when it initializes on its default route, so the routing session reserves that machine before the asynchronous initialization instead of only after it, and a concurrent call cannot place the same agent on another machine meanwhile.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c78629f5e5
ℹ️ 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 routable = | ||
| agent.codeExecutionChoices?.some((choice) => choice.environmentType === 'attached') === true; | ||
| if (agent.id && (agent.codeExecutionContext?.environmentType === 'attached' || routable)) { |
There was a problem hiding this comment.
When a lazy child has no usable default codeExecutionContext but has an attached entry in codeExecutionChoices, this adds the child to the attached-agent set while collectAttachedCodeEnvironmentPolicySettings() still has no settings for it. createRun() therefore validates a requested acceptEdits or fullAccess mode against the config-less fallback before the child is routed, which permits only ask, so a fallback-routable child fails run creation even when its alternate machine explicitly permits the requested mode. Include the choice policies in initial approval-mode validation, especially for descriptors whose default route is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6ce0fa: the requested mode is now validated against collectAttachedCodeApprovalPolicies, which includes each agent's default attached route and every attached machine in its codeExecutionChoices, in createRun and in both client checks. A child whose default is unavailable but whose per-call machine permits accept edits no longer fails run creation; full access still requires every machine it may run on to permit it, as for default routes. The agent-keyed policy map used by the PreToolUse hook is unchanged. Covered by tests.
…y Use - Collect each agent's default attached route and each attached machine a parent may route it to per call, and validate the requested approval mode against that set in createRun and both client checks. - A child whose default is unavailable but whose per-call machine permits accept edits no longer fails run creation; full access still requires every such machine to permit it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6ce0faf0b
ℹ️ 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 codeApprovalMode = resolveAttachedCodeApprovalMode( | ||
| this.options.req.body.codeApprovalMode, | ||
| attachedCodeEnvironmentSettings, | ||
| collectAttachedCodeApprovalPolicies(topLevelAgents), |
There was a problem hiding this comment.
When a lazy child has a permissive attached default but a routable alternate machine whose policy can return ask, approval-mode validation now examines both routes, but buildAttachedCodeEnvironmentAdmissionHooks still receives the default-only attachedCodeEnvironmentSettings. On a scheduled run, runCanPause can therefore remain false and skip the Redis/durable-checkpointer requirements; if the model selects the alternate, lazy registration changes the runtime hook to ask only after that preflight, leaving the resulting pause on an unsupported topology. Build admission metadata conservatively from every codeExecutionChoice, not only the default context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab9e7c7: buildAttachedCodeEnvironmentAdmissionHooks now also takes each agent's per-call route policies (collectAttachedCodeRoutePolicies: default route plus every attached codeExecutionChoice), and an agent counts as able to pause for a category when any of those machines asks. The client's scheduled-run admission passes them, so a permissive default no longer hides an asking alternate and the Redis/durable-checkpointer requirements apply. Covered by a test.
| for (const choice of agent.codeExecutionChoices ?? []) { | ||
| addRoute(choice, choice.codeSessionKey); |
There was a problem hiding this comment.
When a graph member is reachable only through a lazy child's subagents.graphs, it exists at startup only in subagentGraphMemberMetadata, which enqueueCodeFilesChildren does not traverse. Consequently this new choice loop never sees that member's codeExecutionChoices, so a historical file-backed skill is not primed for any machine the member may run on and its first code call lacks the historical skill files. Fresh evidence beyond the earlier priming fix is that other run-wide walkers now explicitly traverse subagentGraphMemberMetadata, while this queue still omits it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab9e7c7: enqueueCodeFilesChildren now walks subagentGraphMemberMetadata, so collectCodeExecutionProfileRoutes primes historical skill files for every route (default and per-call choices) of a graph member that only a lazy child initializes. Covered by a test.
| const resolution = await resolveSubagentCodeTargets(paramsFor(agent, flags)); | ||
| const target = selectSubagentCodeTarget(hostArgs, resolution); |
There was a problem hiding this comment.
When a subagent call is canceled while resolveSubagentCodeTargets() is awaiting live worker checks, the outer waitForAbort rejects but does not cancel this promise; once the checks finish, routeTo() still mutates routeByAgent and claims the machine for an execution that never initialized. If the parent run remains active and retries that child on another machine, the stale claim rejects the retry as a route conflict. Pass the call signal into routing or recheck it immediately after each target-resolution await before mutating session state.
AGENTS.md reference: AGENTS.md:L45-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab9e7c7: place rechecks the call's signal after each target-resolution await and throws before touching session state, so a call canceled during the live worker checks never claims a machine and a retry on another machine is not refused. Covered by a test.
- Build scheduled pause admission from every attached machine a parent may route an agent to, so a permissive default does not hide a per-call machine that asks. - Walk lazy graph-member metadata when collecting execution routes, so historical skill files are primed for every machine such a member may run on. - Stop placing a canceled call after its target resolution resolves, so it never claims a machine for an execution that will not initialize.
…ed Member Configs - A call whose parent run was never placed (an allowSelf spawn reuses its routed inputs) inherits the routed claim of its parent agent, which keeps one machine per request. - Never share a graph member initialized for a routed parent's execution through the graph-member cache, so a failed resolution leaves no config behind for a route it gave back. - Race lazy target discovery with the request signal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc783b7bd
ℹ️ 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 claimed = routeByAgent.get(context.parentAgentId); | ||
| return claimed?.routed === true ? (claimed.environmentId ?? undefined) : undefined; |
There was a problem hiding this comment.
When a routed code agent delegates through a non-stateful child that then uses allowSelf, the self run has no childRoutes entry, so this fallback must recover the route through parentAgentId. However, attach() records that non-code child in routeByAgent as routed: false with no code environment, causing this line to return undefined; a code-enabled grandchild invoked by the self-spawn then falls back to its saved/default machine instead of the routed ancestor's workspace. Fresh evidence beyond the separate non-code-intermediary and self-spawn fixes is their composition: the pass-through route exists only for the original child execution and is lost at the self boundary. Preserve the inherited childEnvironmentId per agent for this fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f63b34e: attach now records, per agent, the machines an agent without stateful code passed on, and the parentAgentId fallback uses that when the agent has no routed claim. It follows the pass-through only when the agent passed on exactly one machine this request (a non-code agent may run under differently routed parents); otherwise the call keeps #16756's static inheritance. Covered by a routing test for a self-spawn of a non-code writer, including the ambiguous case.
…Code - Record the machine each agent without stateful code passes on, and let a call from that agent's unplaced run (an allowSelf spawn) follow it when the agent passed on exactly one machine this request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63b34eca5
ℹ️ 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 (placement.target == null && placement.childEnvironmentId != null) { | ||
| const passedOn = passedOnByAgent.get(agentId) ?? new Set<string>(); | ||
| passedOn.add(placement.childEnvironmentId); | ||
| passedOnByAgent.set(agentId, passedOn); |
There was a problem hiding this comment.
When a non-code child is initialized under a routed parent, this records the inherited machine before the lazy child's graph resolution finishes, but settleExecution() only rolls back route holds on cancellation or failure and never removes this passedOnByAgent entry. If that child is later retried under an unrouted parent and self-spawns, parentRouteOf() can recover the stale machine and send a code-enabled grandchild to the workspace from the failed attempt. Track this pass-through per pending execution and commit or discard it with the rest of that execution's routing state.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f3f8b82: a pass-through recorded during a call is now kept with that call's other routing state (grouped by its execution and parent run): settleExecution records it only when the resolution deciding it succeeds and drops it on failure or cancellation; a pass-through outside any call is recorded directly. Covered by the self-spawn routing test, which now fails a first writer resolution and checks that a self-spawn does not follow it.
| if (!isSubagentHostArgsSupported() || flags.statefulCodeSessions !== true) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
With the committed dependency set, this condition always returns early: package-lock.json pins @librechat/agents 4.0.1, while this file says hostArgs was added after 4.0.2, and the new test explicitly mocks SubagentHostArgumentError because the installed SDK does not export it. Consequently descriptors never receive machine or workspace, codeExecutionChoices remains empty, and the per-call selection feature is disabled in every npm ci deployment. Update the package and lockfile to a release that supplies the host-argument API instead of testing only against the mock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the earlier thread on this point: intentional until LibreChat-AI/agents#596 is released. There is no published @librechat/agents version with the host-argument API to bump to yet, so the declarations are gated on the installed SDK exporting SubagentHostArgumentError and the feature is inert on 4.0.x (no schema change, unchanged behavior). The merge order in the description is: release agents#596, then bump @librechat/agents in this PR and drop the local types and the test stand-in.
- Record the machine an agent without code passes on with the rest of its call's routing state: kept when the resolution deciding it succeeds, dropped when it fails, so a self-spawn never follows a failed attempt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3f8b82086
ℹ️ 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 (!contexts.has(agentId)) { | ||
| contexts.set(agentId, toolContext); |
There was a problem hiding this comment.
When a routed lazy child initializes one graph member and then its overall resolution fails or is canceled while loading another member, settleExecution() releases the route claims but leaves the completed member's tool context in contexts. A retry may therefore select another machine, yet this guard preserves the first machine's context; graph members are registered under a synthetic initialization ID, so event-driven tool loading can fall back to this per-agent entry and execute or provision files against the abandoned machine while approval policy reflects the retry's machine. Roll back that entry with the failed resolution or replace it when the prior route was released.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 331c2e7: when a still-pending placement seeds the per-agent tool context, the routing session records it as the entry's owner together with the value it replaced; when that placement's resolution fails or is canceled the entry is restored (or removed), so a retry on another machine seeds its own context instead of inheriting the abandoned one. Covered by a routing test where a routed reviewer and its routed graph member attach and the resolution then fails.
| settleHoldsOf(context?.executionId, true); | ||
| return resolved; |
There was a problem hiding this comment.
When cancellation arrives after child.resolve(context) finishes but before createLazySubagentConfig.resolveAgentInputs performs its post-await abort check, this commits the child and graph-member claims and removes their abort listeners, then the outer resolver throws without exposing the child. A retry in the still-active parent run cannot select another machine because the canceled call's route is now permanent. Fresh evidence after the graph-resolution fix is that settleExecution() still ends before the SDK resolver's cancellation check and input-building steps; keep the claims pending until that outer resolver succeeds.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 331c2e7: lazy descriptors now carry a settle hook, and createLazySubagentConfig runs its whole resolveAgentInputs body (the host resolver, the post-await abort check, resolved-agent registration and input building) inside it. The routing state (routes, per-agent contexts, the shared graph-cache entry) is therefore committed only when the inputs are handed to the SDK and given back if anything before that throws. Covered by a createRun test where the call is canceled right after the host resolver returns, and by the initialize tests, which now select through the hook.
- Wrap the run's whole lazy resolution, through the inputs handed to the SDK, in the descriptor's settle hook, so a cancel after the host resolver returns still gives the call's routes back. - Roll back the per-agent tool context a pending placement set (restoring the entry it replaced) when its resolution fails, so a retry on another machine seeds its own context. - Drop a failed call's shared config from the graph cache through the same settle hook.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 331c2e77dc
ℹ️ 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".
| routedContexts.set(executionId, { agentId, toolContext }); | ||
| if (!contexts.has(agentId)) { | ||
| seed(); |
There was a problem hiding this comment.
When two parallel executions of the same routable child settle on the same machine, the first attachment seeds agentToolContexts, while the second skips this block because the agent key already exists and therefore never becomes a context owner. If the second resolution succeeds and the first later fails or is canceled, cleanup for the first placement deletes the shared entry even though the successful execution remains; callbacks without execution ancestry then fall back through agentToolContexts and can load or provision tools with no context or the primary agent's context. Track every live placement using the entry, or reseed it from a surviving placement before deleting it.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in faded6d: every pending placement that uses an agent's per-agent tool context is now tracked with its own context (whether it set the entry or found it present). A committed placement sets the entry to its own context; a failed one hands the entry to a surviving placement; only when the last user fails is the earlier entry restored (or removed). Covered by a routing test with parallel placements in both orders.
| } | ||
| const routed = routedContexts.get(entry.subagentRunId); | ||
| return routed?.agentId === agentId ? routed.toolContext : undefined; |
There was a problem hiding this comment.
When a per-call-routed child uses allowSelf, the self execution appears as the innermost ancestry entry with the same agent ID but is never added to routedContexts; this immediate return therefore yields undefined instead of continuing to the outer routed execution. With parallel invocations of the same saved agent, the per-agent fallback may belong to a sibling invocation, so the self-spawn can load tools, provision files, or attribute artifacts using that sibling's context rather than its own routed parent's context. Continue scanning matching ancestry entries until a routed record is found.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in faded6d: getToolContext keeps scanning outward past an ancestry entry of the same agent that has no routed record (an unplaced allowSelf run) until it finds the routed run that spawned it. By the one-machine-per-agent rule the enclosing routed run's context is the right one for the self run. Covered by a test.
| const results = await Promise.all( | ||
| authorized.map((selection) => resolveCandidate(params, selections, selection)), | ||
| ); |
There was a problem hiding this comment.
On a cold cache, resolving choices for multiple lazy descriptors concurrently can start more than 32 distinct worker probes here. The shared status poller rejects every request above its global maxConcurrent = 32 ceiling with CodeBridgeStatusError('busy'), and resolveCandidate() catches that rejection and records the healthy machine as unavailable; because descriptor construction also runs in parallel, even agents within the normal per-agent choice limit can nondeterministically lose advertised routes. Queue or concurrency-limit these probes instead of launching all authorized selections at once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in faded6d: resolveSubagentCodeTargets now runs candidate probes through a process-wide limiter of eight in flight, well below the shared status poller's ceiling of 32, so concurrent descriptor construction queues instead of drawing busy rejections that would be recorded as unavailable machines. Covered by a test that resolves twelve machines and asserts the peak in-flight probes.
…et Probes - Track every pending placement that uses an agent's per-agent tool context: a committed one sets it, a failed one hands it to a surviving placement, and only the last failure restores the earlier entry. - Let an unplaced run of an agent (an allowSelf spawn) defer to the routed run of the same agent that encloses it when looking up tool contexts. - Queue live target probes at eight in flight, below the shared status poller's ceiling, so concurrent descriptors never misreport a healthy machine as unavailable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faded6dbec
ℹ️ 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 agent = await waitForAbort(db.getAgentWithVersionCount({ id: memberId }), graphSignal); | ||
| if (!agent || !(await hasSubagentViewAccess(agent, memberId, graphSignal))) { | ||
| skippedAgentIds.add(memberId); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This new path performs database loading, view authorization, routing initialization, and failure policy directly inside legacy /api, rather than leaving this file as wiring. Move the routed graph-member workflow into a TypeScript service under packages/api and inject its dependencies here so the new backend behavior respects the repository's module boundary.
AGENTS.md reference: AGENTS.md:L73-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1a25692: the routed graph-member workflow (shared-member reuse, skip list, view check, execution identity, failure policy) is now createRoutedGraphMemberLoader in packages/api/src/agents/lazySubagents.ts, with unit tests; initialize.js only injects the database read, view check, initializer and fatal-error predicate.
| const SUBAGENT_TARGET_PROBE_CONCURRENCY = 8; | ||
| const probeTarget = createConcurrencyLimiter(SUBAGENT_TARGET_PROBE_CONCURRENCY); |
There was a problem hiding this comment.
Under process load where 25 or more non-target worker-status polls are already active, this independent eight-slot limiter can still push createCodeBridgeStatusPoller past its shared maxConcurrent = 32 ceiling. The excess polls are rejected as busy, and resolveCandidate() catches that as an unavailable target, so healthy machines can still disappear nondeterministically; queue at the shared poller or retry busy rather than assuming eight slots reserve global capacity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. The 32-request ceiling is the process-wide back-pressure valve of the shared status poller, and every status consumer (default-route resolution, #16756 inheritance, workspace validation) already treats busy as unavailable rather than queueing or retrying; making per-call targets the one caller that retries would let it keep pressure on a saturated poller. The limiter keeps this path's own contribution well below the ceiling, and a machine dropped under saturation is reported as unavailable with a model-visible, retryable error. It is never misrouted.
| const results = await Promise.all( | ||
| authorized.map((selection) => | ||
| probeTarget(() => resolveCandidate(params, selections, selection)), | ||
| ), |
There was a problem hiding this comment.
When a request is canceled while one of these probes is queued, waitForAbort() in initialize.js only rejects the caller; no AbortSignal reaches resolveSubagentCodeTargets() or the limiter. Fresh evidence on this head is the new process-wide queue: its stale thunks still execute, so canceled large graphs can occupy all eight slots and delay unrelated requests through multiple worker timeouts. Thread the signal through and discard queued work before it starts.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1a25692: target resolution now takes the request or call signal (describe gets the request signal, place the call's). A probe that reaches the front of the queue after cancellation is skipped without touching the poller, and the resolution rejects with the abort reason. Covered by a test.
| if (replace || !contexts.has(agentId)) { | ||
| contexts.set(agentId, toolContext); |
There was a problem hiding this comment.
When a pending lazy placement has registered an owner and the same agent is then initialized as an unrouted graph member, that graph-member placement has no execution IDs and is committed before register(true) writes its context here. Fresh evidence beyond the earlier parallel-placement fix is that this committed writer is not added to owner.users; if the older pending placement later fails, settleContext() restores its saved previous value or deletes the entry, clobbering the live graph member's context and leaving its tool loading or provisioning with stale/no state. Update or retire the pending ownership record whenever a committed placement replaces the entry.
AGENTS.md reference: AGENTS.md:L45-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1a25692: when a committed placement (one outside any call) installs the per-agent entry, any pending ownership record for that agent is retired, so a later failure of the older pending placement cannot restore or delete over it. Covered by a test.
| const resolveTargets = async (params: SubagentCodeTargetParams): Promise<SubagentCodeTargets> => { | ||
| const resolution = await resolveSubagentCodeTargets(params); |
There was a problem hiding this comment.
In a shared-file run every target returned by resolveSubagentCodeTargets() is attached and is discarded immediately afterward, but the code still performs and awaits all live worker probes first. Fresh evidence beyond the earlier shared-file routing fix is this post-resolution filtering order: an offline attached machine can impose two 10-second status attempts, repeated in eight-wide batches across lazy descriptors, delaying a run that cannot use any of those routes. Return no targets before probing when sharedRunFiles is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1a25692: with run files shared, the routing session returns no targets before probing (every per-call target is an attached machine), so neither descriptors nor calls wait on worker checks. The shared-file test now also asserts no status request is made.
- Move the routed graph-member load (shared reuse, skip list, view check, execution identity, failure policy) into createRoutedGraphMemberLoader; the initializer only injects its dependencies. - Thread the request and call signals into target resolution so queued probes are skipped once canceled. - Probe nothing in shared-file runs, where every per-call target is attached and unusable. - Retire pending tool-context ownership when a committed placement installs the per-agent entry.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a25692818
ℹ️ 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".
| } catch (error) { | ||
| if (isFatal(error, signal)) { | ||
| throw error; | ||
| } | ||
| logger.error(`[lazySubagents] Error initializing routed graph member ${memberId}:`, error); | ||
| return null; |
There was a problem hiding this comment.
When initialize rejects with an ordinary operational error, such as a provider, tool-loading, or database failure, isFatalAgentInitializationError returns false here and the loader converts the failure to null. resolveGraphSubagentsFor then merely skips the incomplete graph, allowing the routed parent to run successfully without its configured graph instead of surfacing the outage. Propagate unexpected initialization failures; reserve null for documented absence or access denial.
AGENTS.md reference: AGENTS.md:L116-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this for parity with the non-routed path. A graph member loaded for an unrouted parent (loadGraphMemberOnce, unchanged from dev) already logs a non-fatal initialization failure and returns null, and resolveGraphSubagentsFor then skips that incomplete graph with a warning while the parent still runs. The routed loader follows the same policy, so whether a graph is skipped or fails the call does not depend on whether its parent was routed per call. Changing that policy for graph members belongs in its own change covering both paths.
| if (!subagentCodeRouting.isRouted(context.executionId)) { | ||
| graphMemberConfigsById.set(config.id, config); | ||
| sharedConfigByCall.set(context, config); |
There was a problem hiding this comment.
When one default-placed lazy resolution reaches this insertion while another routed parent concurrently loads the same agent as a graph member, the second resolution can reuse this config before the first call settles. If the first call then fails or is canceled, forgetSharedConfig removes the entry and settleExecution releases its route and tool context, but the second parent has already retained the borrowed config without acquiring its own hold; a later retry can therefore move that agent to another machine while the surviving graph still executes with the abandoned workspace and policy. Keep pending configs private until settlement succeeds, or make every concurrent borrower retain the route independently.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f274737: a default-placed lazy child's config is no longer written to the shared graph cache during its resolution. It is published from the settle hook's onCommit, after the call's routes commit, so no concurrent graph can borrow a config whose route may still be given back (a child that appears in its own graph resolves to its own config directly). Covered by an initialize test where an unsettled resolution is not reused by a later routed graph and a settled one is.
| const routeHashes = [agent?.codeExecutionContext, ...(agent?.codeExecutionChoices ?? [])] | ||
| .filter((context): context is CodeExecutionContext => context?.statefulSessions === true) | ||
| .map((context) => hashCodeExecutionTarget(agentId, context)); |
There was a problem hiding this comment.
When a worker reconnects or a workspace instance changes after lazy descriptors are built but before a subagent call, place() correctly re-probes and executes against the new context, but these hashes still come from the descriptor's original codeExecutionChoices; resolved children are registered in policy/tool registries without replacing the objects later passed to captureCodeExecutionApprovalBinding. A code tool can therefore pause after executing against the new route while persisting a binding for the old route, and resume initialization then reconstructs the new route and rejects the user's approval as changed even though no change occurred after the pause. Update the binding metadata with the revalidated selection before the pause, or capture the resolved run contexts directly.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The choices are bound exactly like the default route: both come from the descriptor built at request start, and the resume check rebuilds descriptors and compares. A worker whose routing identity changes between request start and the pause therefore fails the resume closed for defaults and choices alike; the user re-runs. Binding the call-time route instead would need a per-execution binding (the SDK pause carries no execution-scoped target), and a spurious fail-closed on a mid-turn reconnect is the conservative outcome for an approval gate.
…mmits - Publish a default-placed lazy child's config to the shared graph cache from the settle hook's commit, so no concurrent graph can borrow a config whose route may still be given back. - Resolve a child's own appearance in its graph to the child config itself.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
Dev's per-call subagent machine selection (#16757) changed the approval-mode resolver to validate against every attached policy a run may execute under, including machines a parent may route a subagent to per call. The no-target branch, persisted mode, and preserved-field rule now read that policy list (collectAttachedCodeApprovalPolicies), so a turn is treated as having no attached target only when no agent runs, or may be routed, on one.
1 participant
Summary
A parent agent working in an attached machine's workspace could delegate to a subagent that then ran somewhere else. In the incident behind this change, Lia worked in a BYOM workspace for
danny-avila/agentsand delegated a review to the PR Reviewer subagent, which started in the Code API checkout and could not see the PR. #16756 makes children inherit their parent's machine by default when allowed. This PR lets the parent choose explicitly, per subagent call.Each lazy subagent now advertises optional
machineandworkspacearguments on thesubagenttool, as enums of exactly the choices that child can reach right now: machines admitted in the conversation's sealedcodeWorkspaces, on the child's default or allowlist, present in the requesting principal's tenant-scoped machine list, and served by a ready worker. A child the conversation pinned to a machine offers only that machine; a child with no reachable machine declares nothing and its schema is unchanged.workspaceis declared only when each reachable machine's workspace ID is distinct (each machine has exactly one workspace in a conversation, so it is an alternative way to name the same choice).When the parent passes a choice, the child's resolver re-validates it against the current request (allowlist, principal list, sealed decision, live worker and workspace) before initializing the child, and refuses anything no longer reachable with a model-visible error naming the argument, for example
Subagent error: The requested "machine" is unavailable right now. Omit "machine" to let the host choose, or pass a different value.There is no silent fallback. Omitting both arguments keeps #16756's inherit-by-default routing, extended so a child also inherits the machine a per-call-routed parent runs on (passing through agents that run no code).A child whose default workspace is unavailable keeps #16756's guard (listed as unavailable, never runs) unless it has per-call choices; then it stays callable, its description asks the parent to pass a listed
machine, and an omitted call is still refused with the same workspace error.A subagent keeps one machine for the whole request, matching #16756's "one route per subagent" rule: a later call that omits the machine follows that subagent's earlier per-call route, and a call that would move it elsewhere is refused. The route is claimed when the call is placed (before the asynchronous initialization), including for a default route and for graph members. A call's claims, the per-agent tool context it sets, and its config's entry in the shared graph-member cache stay pending until the run hands the child's inputs to the SDK (a
settlehook on the lazy descriptor wraps the whole selection); a failed or canceled selection gives them back unless another execution still holds them. Run-wide state keyed by agent (the attached-machine permission policy, tool contexts) therefore always matches where the child runs; graph members a routed child initializes are registered with the policy of the machine they actually run on, and a routable child that resolves off attached machines drops its provisional attached-machine policy.When run file sharing is requested for the root, no attached machine is offered, probed or accepted per call, since shared children must run on managed code. A call whose parent run was never placed (an
allowSelfspawn) follows its parent agent's routed or passed-on machine.Builds on #16756 (merged into
dev). Depends on LibreChat-AI/agents#596 (the SDK host-argument extension point).How it works
createSubagentCodeRoutingis a request-scoped session inpackages/apithat owns host-argument interpretation, inheritance, the unavailable-default decision, placement validation, route claims and per-execution state;initialize.jsonly constructs it and passes values through. Live target probes go through a process-wide queue of eight, below the shared status poller's ceiling, and honor the request or call signal.The routed child is pinned by narrowing its agent document to the chosen machine (
code_environment_idand a one-entry allowlist). Every existing routing site (initializeAgent,ToolServicetool loading, provisioning) reads that document, so neither #16756's inheritance nor another admitted selection can move it, and no routing function needed a new parameter. A routed child's tool context is also recorded per execution and consulted first by tool loading,toolEndCallbackartifact handling and file provisioning (the run-file provisioning wrapper now forwards the execution context).captureCodeExecutionApprovalBindingfolds an agent's default route and itscodeExecutionChoicesinto one target, so a paused approval fails closed if any of those targets changed while it waited, and each agent contributes at most one binding entry.Preflight decisions consider every machine a child may be routed to: the requested approval mode is validated against each attached default and choice (
collectAttachedCodeApprovalPolicies; full access still requires all of them to permit it), and scheduled-run pause admission counts an agent as able to pause when any of its routes asks (collectAttachedCodeRoutePolicies). Historical skill files are primed for every route, including those of graph members only a lazy child initializes.Type of change
Testing
Tested environments/configuration: Windows, Node 24.16.0, mongodb-memory-server; attached machines simulated with a control plane and worker status responses.
Automated tests:
packages/api/src/code/targets.spec.ts: enum construction per child (admitted + allowlisted + principal + ready only; allowlist exclusion; selection disabled keeps only the default; machine missing from the principal list; allowed-but-unadmitted machine never added; offline worker and missing workspace reported as unavailable; conversation-pinned child; sealed decision wins over a conflicting request); same inputs give the same route and session key; declaration shape and description length; selection and every refusal kind; re-validation on use (offline worker after describe, revoked allowlist entry, principal that lost the machine); omitted args leave the agent on its default; a routed child stays on its machine when its parent's machine would otherwise be inherited; inheritance from a routed parent, including through an agent without code; one machine per subagent per request; unavailable default refused only when no per-call route applies; approval binding covers and detects changed choices and folds 32 choices into one target; attached-policy install and skill priming for choice-only children; per-execution tool contexts for concurrent siblings; a route that initialization did not land on fails closed.packages/api/src/code/targets.integration.spec.ts(mongodb-memory-server): a machine registered through the real code-environment registry and merged into the principal's config is offered to its owner, refused to another principal, and refused across tenants.packages/api/src/code/targets.spec.ts(concurrency and lifecycle): concurrent omitted-machine calls from differently routed parents settle on one machine; workspace-only conflicts nameworkspace; a call canceled during target resolution or before attaching claims nothing; a failed initialization gives its machine back unless another call holds it; an attached claim is kept; shared-file runs offer and accept no attached machine.api/server/services/Endpoints/agents/initialize.spec.js: a descriptor resolved withmachineinitializes the child pinned to that machine, a later call without arguments stays there, and an unadmitted machine is refused before initialization; a routed reviewer's graph members land on its machine; a lazily loaded graph member's default is held while it initializes; a failed default initialization does not block a retry on another machine.packages/api/src/agents/__tests__/run-codeTools.test.ts:hostArgsreaches only the lazy configs that declare choices; throughcreateRun's realPreToolUsehooks, a routed graph member takes its machine's policy and a routable child that resolved on managed code drops the attached policy.packages/api/src/agents/hitl/byom.spec.ts: approval-mode validation and pause admission over per-call machines.packages/api/src/agents/codeFilesSession.spec.ts: lazy graph-member choices are primed.packages/api/src/files/provision/callback.spec.ts: a routed child provisions into its own execution's context.npx tsc --noEmitinpackages/api, the--isolatedDeclarationsdeclaration check on touched files, ESLint, Prettier and import sorting on touched files; the apiinitializeandresumespecs pass.The SDK-side behavior (schema, validation, execution binding, replay digest) is covered in LibreChat-AI/agents#596.
Screenshots / recordings
No user-facing change.
Risk / compatibility
SDK dependency and merge order. The feature needs
@librechat/agentswithSubagentConfig.hostArgs,SubagentResolveContext.hostArgsandSubagentHostArgumentError(LibreChat-AI/agents#596, the first release after 4.0.2). This PR does not bump the dependency: the declarations are gated on the installed SDK exportingSubagentHostArgumentError, and the host-argument types are declared locally, so on 4.0.x nothing is advertised and behavior is unchanged. Order: (1) merge and release agents#596, (2) bump@librechat/agentsin this PR to that release, and drop the local types and the test stand-in for the SDK error class. #16756 is already merged intodev.Approval bindings. Bindings now also cover per-call choices, so a pause whose child had alternative machines fails closed if one of those machines changes (or goes offline) while the approval waits. A pause captured by a replica without this change and resumed on one with it also fails closed when the child has choices; the user retries. Agents without choices keep their existing binding entry.
Approval modes. A requested full-access mode now also requires every attached machine a child may be routed to per call to permit it.
One machine per subagent per request. A parent cannot fan the same subagent out to two machines in one turn; the second call is refused with a
machineerror. Different subagents can run on different machines.Not included. A
worktreeargument (.worktrees/<name>) is left out: there is no host-side default working directory to route a child into a linked worktree, and workspace tools already accept a per-commandcwd, so the parent can name the worktree in the task description. A child that is also a root (handoff/parallel) agent is initialized eagerly for that role and keeps its existing route, since the SDK only accepts host arguments on lazy configs.Checklist