feat: add host-owned context for subagent executions - #539
Conversation
Expose trusted execution identity and prepare/complete adapters for child runs, isolate code session partitions, and retain authorized inputs across failed code calls.
|
@codex review |
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: c5f3f99e8a
ℹ️ 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".
| */ | ||
| export interface BaseHookInput { | ||
| /** SDK-owned child lineage, including distinct identities for concurrent self-spawns. */ | ||
| executionContext?: SubagentExecutionContext; |
There was a problem hiding this comment.
When a child graph triggers summarization, its PreCompact and PostCompact calls in src/summarization/node.ts still construct hook inputs without executionContext (lines 920-929 and 1327-1339). Consequently, hosts relying on this newly exposed lineage cannot correlate or authorize compact lifecycle hooks for concurrent copies of the same saved agent, even though tool, preemption, and subagent start/stop hooks receive it. Thread the graph's subagent execution context through both compact hook inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Propagated the canonical child execution context through both PreCompact and PostCompact hooks in commit 1083640, with focused compaction-hook coverage.
| } catch { | ||
| return createSubagentFailure('Subagent result delivery failed.'); |
There was a problem hiding this comment.
When complete() fails in a HITL-enabled run, this converts the failure into an ordinary result, and Graph.ts returns only result.content from the subagent tool, so ToolNode treats it as a successful terminal ToolMessage and persists it through persistSettledToolOutput. That settlement clears the retained completedResult, meaning replaying the same call only returns the cached “delivery failed” output without retrying complete; retrying with a new model-generated tool-call ID instead executes the child and its side effects again. Preserve this as a retryable delivery state rather than settling it as the subagent's output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Kept delivery failures out of durable settlement in commit 1083640. Error outputs are not settled while a completed child result is retained, and the regression test proves complete() retries without re-running the child.
| retainCodeSessionInputs( | ||
| this.sessions, | ||
| this.codeSessionKey, | ||
| request.codeSessionContext | ||
| ); |
There was a problem hiding this comment.
When an event-driven host lazily populates request.codeSessionContext and then uses the documented batch.reject(error) path for a fatal transport or execution failure, control exits before storeCodeSessionFromResults is called, so this new retention helper never runs and the authorized input files disappear from the session. The eager path has the same gap because resolveEagerEventExecution throws before copying the mutated request context. Retain the request contexts in the rejection path as well so the next code call still receives files that were successfully provisioned before the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retained host-provisioned code-session inputs on dispatched and eager rejection paths in commit 1083640, with focused tests for both failure modes.
|
@usnavy13 I'm going to take over this PR since it's directly related to recent subagent work introduced over v0.8.8, hoping we can include it in |
|
if you can help test after my changes, that would be great |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1083640a49
ℹ️ 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".
| parentToolCallId, | ||
| parentConfigurable, | ||
| }); | ||
| if (execution.completedResult != null && settled.output.status === 'error') { |
There was a problem hiding this comment.
When complete() rejects, execute() returns createSubagentFailure, but the subagent wrapper in Graph.ts returns only result.content; runTool() consequently creates a ToolMessage with status: 'success'. This guard therefore never fires in the real ToolNode path, so the delivery-failure text is still durably settled and subsequent retries cannot call complete() again. Fresh evidence in the current head is that the new regression test manually passes an error ToolMessage to persistSettledToolOutput instead of exercising the wrapper that produces the successful status.
AGENTS.md reference: AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ccac63: retryable delivery failures now throw through the subagent tool wrapper so ToolNode creates an error ToolMessage and leaves the completed child result unsettled for complete() retry.
| runId: (config.configurable?.run_id as string | undefined) ?? '', | ||
| threadId: config.configurable?.thread_id as string | undefined, | ||
| agentId: this.agentId, | ||
| executionContext: this.executionContext, |
There was a problem hiding this comment.
When a child uses the local programmatic tool to invoke an inner tool, this value is placed into hookContext, but ProgrammaticHookContext does not declare it and applyPreToolUseHooksForBridge() does not copy it into the inner PreToolUse input. Those inner hooks therefore still receive no child lineage, preventing hosts from correlating or authorizing calls from concurrent copies of the same saved agent; extend the bridge context type and forward the field when executing the hook.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ccac63: ProgrammaticHookContext now carries executionContext and the local bridge forwards it to inner PreToolUse hooks, with focused coverage.
| [eagerResults, dispatchedResults] = await Promise.all([ | ||
| eagerResultsPromise, | ||
| dispatchPromise, | ||
| ]); |
There was a problem hiding this comment.
In a mixed batch containing eager and dispatched calls, Promise.all rejects as soon as either side fails, so this catch snapshots requestMap while the other execution may still be provisioning codeSessionContext. If that surviving execution attaches files afterward, its finally only copies them into the request and nothing invokes retention again, causing those authorized inputs to disappear on the next code call. Fresh evidence in the current head is this mixed-path race; the added tests cover eager and dispatched rejection only in isolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ccac63: mixed eager/dispatched outcomes now await both peers before retaining provisioned request inputs, with a mixed-path race regression test.
| member.codeSessionKey = sessions.codeSessionKey; | ||
| member.initialSessions = sessions.initialSessions; |
There was a problem hiding this comment.
When an interrupted child resumes in the same SubagentExecutor, prepare() may return updated initialSessions while retaining the same codeSessionKey, but this assignment only mutates the newly created childPlan. The executor subsequently reuses execution.activeRun.graph, and seedChildGraphSessions() runs only when no cached child exists, so the refreshed files never reach the live graph's session map and resumed code tools continue with stale inputs. Merge the prepared sessions into the cached graph as part of reauthorization, while preserving its accumulated runtime sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ccac63: prepared session seeds are merged into cached child graphs as well as new graphs, preserving accumulated runtime sessions through the existing merge helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ccac6321e
ℹ️ 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".
| langfuse: this.langfuse, | ||
| tokenCounter: this.tokenCounter, | ||
| usageSink: this.usageSink, | ||
| subagentContext: this.subagentContext, |
There was a problem hiding this comment.
When run_in_background is used and subagentContext.complete() fails transiently, forwarding the adapter into this detached executor causes completeHostContext() to return a retryable result with error, which executeDetached() immediately throws. The task store then records a terminal error and reuses that task for the parent call's idempotency key, while this detached executor—and its retained completedResult—is cleared. Repeating the same background call therefore never retries complete(), leaving successfully completed child work permanently undeliverable; retain the completed result in the task lifecycle or provide a delivery-only retry path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c0e724d. Detached execution now retries only the host delivery step against the retained completed execution, with bounded exponential backoff, so transient completion failures do not terminalize the task or rerun the child.
| for (const file of context.files) { | ||
| if (!file.id || !file.name || !file.storage_session_id) continue; | ||
| const identity = fileIdentityKey(file); | ||
| if (identities.has(identity) || names.has(file.name)) continue; |
There was a problem hiding this comment.
When an event handler lazily provisions a newly uploaded input whose filename already exists in the session but whose storage session or file ID differs, this name check discards the new reference. This is especially visible when the execution errors or returns no artifact: the current call used the refreshed file, but the next code call is populated with the stale same-name session entry instead. updateCodeSession() already treats same-name artifact files as replacements, so request-side retention also needs to distinguish a genuinely refreshed input from an older same-batch snapshot rather than unconditionally preferring the existing name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c0e724d. Retention now replaces a same-name file when its stable storage/session identity changed, and request inputs are retained before successful artifacts are merged so a later stale request snapshot cannot overwrite a fresh output.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0e724d98c
ℹ️ 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 replacementIndex = indexByName.get(file.name); | ||
| if (replacementIndex !== undefined) { | ||
| indexByIdentity.delete(fileIdentityKey(files[replacementIndex])); | ||
| files[replacementIndex] = { ...file }; | ||
| indexByIdentity.set(identity, replacementIndex); |
There was a problem hiding this comment.
When a batch contains multiple code calls, every request is initially seeded from the same session snapshot. If the host replaces a same-name file in an earlier request but leaves a later request's snapshot unchanged, this unconditional name-based replacement processes the later stale identity afterward and restores the obsolete file, so the next code call receives stale input. Fresh evidence in the current head is that retention now iterates all requests in insertion order while the new regression test covers only a single request; distinguish host-refreshed contexts from the original batch snapshot rather than letting the last request win.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 015ddc8. Request retention now captures the pre-batch session identities and ignores unchanged baseline entries, so an earlier host-refreshed same-name identity cannot be reverted by a later untouched request snapshot. Added a two-call regression.
| while (result.retryableDelivery === true) { | ||
| if (deliveryRetries > 0) { | ||
| await sleep( | ||
| Math.min(100 * 2 ** Math.min(deliveryRetries - 1, 6), 5_000) | ||
| ); |
There was a problem hiding this comment.
When complete() consistently rejects, this loop caps only the delay and never the number of attempts, so a host-provided SubagentTaskStore whose runtime signal has no automatic deadline leaves the task running forever; even the default store holds a running slot and retries the failing host projection for its full 30-minute timeout. A completion-service outage can therefore exhaust detached-task capacity while retaining each executor's completed transcript and hook registrations. Add a finite retry budget or an executor-owned deadline before terminalizing the delivery failure.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 015ddc8. Detached delivery is now capped at three attempts (with backoff before the final attempt), after which the task terminalizes and releases retained executor state. Added persistent-failure coverage asserting one child run, three delivery attempts, and terminal error state.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 015ddc8e18
ℹ️ 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 baselineIdentityByName = new Map( | ||
| (existing?.files ?? []).map((file) => [ | ||
| file.name, | ||
| fileIdentityKey(file), | ||
| ]) |
There was a problem hiding this comment.
When concurrent invoke() calls share this ToolNode, this “baseline” is read only after the current batch settles. If batches A and B are both seeded with file identity X, B refreshes it to Y and finishes first, then A finishes with its unchanged X snapshot, A observes Y as its baseline and misclassifies X as a refresh, reverting the retained file to X. Fresh evidence in the current head is that the baseline is derived from the mutable session map inside this post-execution helper rather than captured when each request batch is created; thread a per-batch pre-dispatch identity snapshot into retention.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c79b98f. The identity baseline is now captured from the session map when each event batch is constructed, before dispatch, and threaded through both rejection and result retention. Added a concurrent out-of-order batch regression.
| request.name === '' || | ||
| (!this.participatesInCodeSession(request.name) && | ||
| request.name !== Constants.SKILL_TOOL) |
There was a problem hiding this comment.
When an event-driven host refreshes codeSessionContext while handling read_file, this filter discards that request even though request construction explicitly attaches the current code-session context to read_file for sandbox fallback. Since a successful read normally has no code artifact to update the session afterward, a same-name refreshed file remains only on the transient request and the next code call receives the stale stored reference. Include Constants.READ_FILE in request-side retention while continuing to exclude it from artifact-driven session updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c79b98f. Request-side retention now includes read_file while artifact-driven updates remain limited to session-writing tools. Added a read_file refresh followed by code execution regression.
| ); | ||
| } | ||
| deliveryAttempts += 1; | ||
| result = await executeAttempt(); |
There was a problem hiding this comment.
When complete() never settles—for example, a host transport hangs without honoring the supplied abort signal—executeAttempt() never returns, so the three-attempt condition is never evaluated again. The default task store can mark the task timed out and abort its runtime, but this coroutine remains suspended and its finally cleanup never runs, retaining the completed transcript, detached executor, and registered hooks indefinitely; race each delivery attempt against the runtime signal or an executor-owned timeout. Fresh evidence in the current head is that the new bound counts only resolved/rejected attempts and does not bound an in-flight delivery call.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c79b98f. Each host completion promise is raced against the detached runtime signal, with listener cleanup, so task timeout/cancellation unwinds the coroutine and clears retained graph/hooks even if complete() never settles. Added hanging-completion timeout coverage.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c79b98fce0
ℹ️ 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 requestMap = new Map(plan.allRequests.map((r) => [r.id, r])); | ||
| const codeSessionBaselineByName = new Map( |
There was a problem hiding this comment.
When eager event execution is enabled, its actual request is created earlier in stream.ts and can be seeded with file identity X; if another concurrent batch refreshes the shared session to Y before this ToolNode consumes that eager record, this line captures Y as the baseline, while the finally at lines 4251-4253 later replaces the planned request context with the eager request's unchanged X. Request retention then misclassifies X as a refresh and restores the stale file. Capture the identity baseline alongside each eager request when it is created, or otherwise compare consumed eager contexts against their request-time baseline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8e48935. Eager execution records now retain their request-creation identity baseline, and ToolNode uses that per-request baseline instead of the later shared-session snapshot when consuming eager results. Added a regression where the shared session refreshes before the eager record is consumed.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e489356ee
ℹ️ 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".
| codeSessionBaselineByName: new Map( | ||
| (entry.request.codeSessionContext?.files ?? []).map((file) => [ | ||
| file.name, | ||
| `${file.storage_session_id}\0${file.id}`, | ||
| ]) |
There was a problem hiding this comment.
When the eager event handler synchronously replaces request.codeSessionContext while handling ON_TOOL_EXECUTE, dispatch has already started at lines 850-859 before this map is created, so the refreshed identity is recorded as the baseline and later skipped by retainCodeSessionInputsFromRequests; the next call therefore keeps the stale or concurrently written session file instead of the host-provisioned input. Fresh evidence in the current head is that the regression test manually constructs an eager record with the desired pre-dispatch baseline rather than exercising this actual stream ordering; snapshot each entry before calling safeDispatchCustomEvent.
AGENTS.md reference: AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c1efd7a. Eager baselines are now snapshotted for every planned entry before safeDispatchCustomEvent is called, so synchronous host mutation cannot redefine the baseline. Added an actual ChatModelStreamHandler regression that mutates the eager request during dispatch.
| this.sessions?.get(this.codeSessionKey) as | ||
| | t.CodeSessionContext | ||
| | undefined | ||
| )?.files ?? [] | ||
| ).map((file) => [file.name, fileIdentityKey(file)]) |
There was a problem hiding this comment.
When a stored FileRef omits its optional storage_session_id, getCodeSessionContext() injects the execution session as the fallback, but this baseline hashes the raw stored ref as \0<id>. The unchanged request is therefore misclassified as a refresh; if another concurrent batch replaces the same filename before this batch settles, retention writes the old request snapshot back over that replacement. Build each baseline from the normalized request context (or apply the same session-id fallback here) so supported legacy/inline refs cannot reintroduce the stale-batch race.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c1efd7a. Normal batch baselines are now derived per request from the normalized injected codeSessionContext rather than raw stored FileRefs, preserving the execution-session fallback for legacy or inline refs. The new stream regression starts from a ref without storage_session_id.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1efd7abaf
ℹ️ 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 (execution.request.codeSessionContext != null) { | ||
| request.codeSessionContext = execution.request.codeSessionContext; |
There was a problem hiding this comment.
When an eager execute_code/bash request has its codeSessionContext refreshed by the host but the final tool name or arguments differ, takeMatchingEagerEventExecution() replaces the original record with a synthetic error execution whose request is the final request. This finally block therefore copies that final request rather than the host-mutated eager request, so request retention never sees the newly authorized files and the next code call is seeded with stale session inputs. Preserve the original eager request and its baseline on the mismatch error path while still refusing to rerun the tool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f5b0b5. The mismatch error execution now preserves the original eager request and its request-time baseline while still using the final identity for the synthetic failure and refusing redispatch. Added a code-session regression with a host-refreshed eager input and changed final tool identity.
| }; | ||
| let preparedContext: PreparedSubagentContext | undefined; | ||
| try { | ||
| preparedContext = await this.subagentContext?.prepare(hostContextInput); |
There was a problem hiding this comment.
When a host-backed prepare() call never settles—for example, an authorization service hangs while ignoring the supplied signal—this direct await prevents both foreground cancellation and detached task timeouts from unwinding the execution. The child graph never starts, but the executor and any detached hook/task state remain retained indefinitely because control cannot reach their cleanup paths. Use the same abort race applied to complete() so aborting childSignal rejects the preparation wait even when the adapter does not cooperate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f5b0b5. Host preparation is now raced against childSignal through the same listener-cleaned abort helper used for completion. Added a non-cooperative prepare regression that waits until preparation starts, aborts, and verifies no child graph is retained.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f5b0b5a17
ℹ️ 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".
| request: execution.request, | ||
| codeSessionBaselineByName: execution.codeSessionBaselineByName, |
There was a problem hiding this comment.
When a host refreshes the codeSessionContext of an eager execute_code, bash, or read_file request but final materialization changes the name to a non-session tool, this preserves the original request only inside the synthetic execution. The finally path copies its context onto the final request, but retainCodeSessionInputsFromRequests() filters using that final request's name and therefore skips the refreshed files. Fresh evidence is that the new regression test changes only between execute_code and bash, both of which pass that filter; carry the original tool's retention eligibility through the mismatch path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2350fe. The mismatch path now marks the final request for code-session input retention when the original eager tool was session-eligible, including mismatches to non-session tools. The regression now changes to a plain weather tool.
| request, | ||
| request: execution.request, | ||
| codeSessionBaselineByName: execution.codeSessionBaselineByName, | ||
| promise: Promise.resolve({ |
There was a problem hiding this comment.
When the mismatch is detected while the original ON_TOOL_EXECUTE handler is still asynchronously provisioning files, this immediately resolved synthetic promise lets the finally path and request retention run before that handler updates execution.request.codeSessionContext. The later host refresh is then never copied into the session. Fresh evidence is that the new regression test pre-populates the refreshed context before invoke() rather than delaying the mutation behind the eager execution promise; settle the synthetic mismatch only after the original execution finishes, while continuing to discard its tool result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2350fe. The synthetic mismatch outcome now waits for the original eager execution promise to settle before discarding its result, allowing asynchronous host provisioning to finish. The regression delays the request mutation until after ToolNode invocation begins.
| : await awaitWithAbort( | ||
| Promise.resolve(this.subagentContext.prepare(hostContextInput)), | ||
| childSignal |
There was a problem hiding this comment.
When childSignal aborts during this new race, awaitWithAbort() rejects into the surrounding catch, which converts cancellation into an ordinary createSubagentFailure. The graph's subagent wrapper only throws retryableDelivery failures, so ToolNode produces a successful ToolMessage and, in a HITL-enabled run, durably settles the generic access-denied text; replaying the same call then returns that cached output without re-running preparation. Fresh evidence is that the cancellation regression invokes SubagentExecutor directly and never exercises the wrapper or durable settlement path; rethrow an aborted signal (or otherwise mark it non-settleable) while keeping genuine authorization failures generic.
AGENTS.md reference: AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2350fe. Preparation cancellation is now rethrown from the authorization-failure catch, so aborted calls remain errors instead of successful access-denied results. Cancellation tests now assert rejection after preparation begins.
| } | ||
| if (!changed) return; | ||
| sessions.set(sessionKey, { | ||
| session_id: existing?.session_id ?? context.session_id, |
There was a problem hiding this comment.
When a session already exists and the host replaces a request's codeSessionContext with a different execution session_id, a rejection or result without an artifact retains the refreshed files but this expression keeps the obsolete session ID. The next code call therefore sends the new file references while continuing the old sandbox execution session, even though ToolCallRequest.codeSessionContext.session_id is explicitly the transient execution session. Use the accepted refreshed context's session ID when persisting these request-side inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2350fe. Accepted request-side refreshes now persist their supplied execution session ID together with their files. Both rejection and eager-mismatch regressions assert the refreshed session ID.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2350fe474
ℹ️ 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 (baselineIdentityByName?.get(file.name) === fileIdentityKey(file)) { | ||
| continue; | ||
| } | ||
| sessionId = context.session_id; |
There was a problem hiding this comment.
When an event handler replaces codeSessionContext.session_id but keeps the same file identities (or supplies no files), sessionId is never assigned because it is set only after detecting a changed file, and the method returns without updating the session. The next code call is therefore injected with the obsolete transient sandbox execution ID even though the host refreshed it; compare the request-time session ID as part of the baseline and persist a changed ID independently of file changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a18f2a1. Requests now carry their normalized request-time execution session ID, and retention persists a changed session independently of file identity changes. Added session-only refresh coverage.
| authorization idempotent for the execution identity. Throwing or aborting during | ||
| preparation prevents child execution and produces a generic failure result. |
There was a problem hiding this comment.
The latest cancellation path rethrows childSignal.reason, so an abort during prepare() rejects SubagentExecutor.execute() and the enclosing tool/run rather than returning a generic failure result. Hosts following this new public guide may consequently handle cancellation as an ordinary result and leave the rejection unhandled; document abort propagation separately from genuine preparation failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a18f2a1. docs/subagent-context.md now distinguishes genuine preparation failures (generic failure result) from signal cancellation (propagated execution error that callers must handle).
| if (childSignal.aborted) { | ||
| throw childSignal.reason instanceof Error | ||
| ? childSignal.reason | ||
| : error; |
There was a problem hiding this comment.
In a HITL-enabled run, rethrowing the preparation abort here is insufficient: the subagent tool wrapper propagates it into runTool(), which catches ordinary abort errors, creates an error ToolMessage, and passes it to persistSettledToolOutput(); because no child result was completed, the new completed-result guard does not apply and the cancellation is durably settled. Replaying the same call therefore returns the cached cancellation instead of re-running prepare(). Fresh evidence in the current head is that the cancellation tests still invoke SubagentExecutor directly rather than the real ToolNode persistence path, so propagate cancellation through that wrapper as a non-settleable run error.
AGENTS.md reference: AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a18f2a1. Durable settlement now skips error outputs when the runnable signal is aborted, keeping preparation cancellation replayable instead of caching it as a terminal tool result.
| const completedChildResult = execution.completedResult; | ||
| if (completedChildResult != null) { | ||
| return this.completeHostContext(hostContextInput, completedChildResult); |
There was a problem hiding this comment.
When a lazy subagent finishes but complete() fails, markCompleted() clears the cached resolved configuration, so retrying the same call reruns resolveAgentInputs before reaching this completed-result check. If that resolver is temporarily unavailable, the retry returns an ordinary initialization failure instead of retrying delivery; the wrapper can then durably settle that failure and clear the retained child result, making the already-completed work permanently undeliverable. Retain the resolved child plan through delivery retries or ensure resolution failures cannot replace a pending completed result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a18f2a1. markCompleted now retains the resolved lazy configuration until durable settlement or invalidation; registry coverage verifies it survives completion and is released after settlement.
|
@codex review Please review the exact current head commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a18f2a13e8
ℹ️ 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".
| Cancellation is different: aborting the supplied signal rejects preparation | ||
| and propagates as an execution error, so callers must handle it as cancellation | ||
| rather than as a normal subagent result. |
There was a problem hiding this comment.
When a host aborts the supplied signal while prepare() is pending, SubagentExecutor.execute() propagates the cancellation, but the immediately preceding sentence still says that aborting during preparation produces a generic failure result. Callers cannot determine whether to handle a rejection or inspect a normal result. Fresh evidence in the current head is that the new propagated-error wording was added without removing or narrowing the conflicting generic-failure statement; reserve that statement for ordinary preparation rejection and describe signal abortion only as propagated cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 00c78b1. The guide now reserves generic failure results for ordinary preparation failures and states separately that signal abortion propagates as cancellation.
| return createSubagentFailure( | ||
| 'Subagent context is unavailable or access was denied.' | ||
| ); |
There was a problem hiding this comment.
When complete() has already failed once and the retry's prepare() transiently throws, this returns an untagged failure before reaching the retained completedResult. Graph.ts consequently returns that failure text normally, ToolNode creates a successful ToolMessage, and persistSettledToolOutput() durably settles it and clears the completed child result; the same call can no longer retry delivery, while using a new call ID can repeat the child's side effects. Fresh evidence after the resolved-config fix is that adapter preparation still reruns ahead of the completed-result check and its failure remains settleable, so retain the completed result through failed reauthorization attempts as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 00c78b1. If completed child work is retained, a preparation/reauthorization failure is now tagged retryableDelivery so the graph wrapper emits an error and durable settlement preserves the completed result. The regression covers delivery failure, reauthorization failure, then successful delivery without rerunning the child.
| if (baselineIdentityByName?.get(file.name) === fileIdentityKey(file)) { | ||
| continue; | ||
| } | ||
| sessionId = context.session_id; |
There was a problem hiding this comment.
When one request in a batch changes only session_id from X to Y and a later sibling refreshes a file while retaining its unchanged baseline session X, this assignment overwrites the explicitly refreshed Y with X merely because the sibling supplied a changed file. The files are retained correctly, but the next code call is sent back to the obsolete execution session. Track an explicit session-ID refresh separately and use a file-bearing request's session only when no request actually changed the session ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 00c78b1. Explicit session-ID refreshes are tracked separately from file-bearing request sessions and take precedence at persistence. Added a sibling-request regression combining a session-only refresh with a later file refresh on the baseline session.
|
@codex review Please review the exact current head commit |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
2 participants
When a host delegates work to concurrent copies of the same saved agent, the agent ID alone cannot identify which child may access a file or tool session.
Run.create({ subagentContext })now gives the host a preparation and completion boundary for each SDK-owned child execution, including nested and graph subagents.This supports run-scoped file sharing in LibreChat #14261. Companion implementation: danny-avila/LibreChat#15848.
prepareauthorizes access before the child graph runs, supplies actual messages and host configuration, and replaces per-member code-session seeds. Preparation failures or cancellation prevent child execution. SDK execution and checkpoint identities remain authoritative.completeprojects a successful result into the text returned to the parent. If delivery fails, the SDK retains the completed work, reauthorizes on retry, and retries delivery without repeating model/tool side effects. Resuming a checkpoint preserves its messages rather than adding the initial context twice.sequenceDiagram participant Parent as Parent Run participant SDK as SubagentExecutor participant Host as Host context adapter participant Child as Child graph and tools Parent->>SDK: Delegate task SDK->>Host: prepare(executionContext, members, signal) Host-->>SDK: Messages, configuration, member sessions SDK->>Child: Run with canonical child lineage Child-->>SDK: Successful result SDK->>Host: complete(context, retained result) Host-->>SDK: Result text with host-owned references SDK-->>Parent: Deliver resultThe adapter contract and resume responsibilities are documented in docs/subagent-context.md. LibreChat supplies file authorization, storage, sandbox ownership, and the sharing tools through this interface. A code-session key partitions the SDK session map; hosts must enforce runtime workspace isolation and reconstruct their authorization on resume. Exported
SUBAGENT_CONTEXT_VERSION = 1lets hosts check support before enabling their integration. Existing callers can omit the adapter.Validation at
c5f3f99e8a60e99d3c29412a67392dc2330ec7c9:b10ebb11; implementation and test sources are unchanged. These are separate from the green automated suite. After the live follow-ups, 81 guarded cases remain unexecuted because they need other provider credentials or unavailable remote protocols, including nine/exec/programmaticcases.Core local reproduction on Linux with Node 24.16.0 and npm 10.5.2:
npm ci npx tsc --noEmit npx eslint "src/**/*.ts" npm run build npm run test:circular-deps npm run check:circular-deps NODE_OPTIONS=--experimental-vm-modules NODE_ENV=test npx jest --roots=src --maxWorkers=2 --testPathIgnorePatterns=title.memory-leak.test.ts --testPathIgnorePatterns=llm.spec.tsThis remains a draft for coordinated SDK release and LibreChat adoption. The package version is unchanged; publishing a version containing this adapter and updating LibreChat's dependency/lockfile remain required before fresh installs can enable the feature.