Repository navigation
⏯️ feat: Enable Graph Subagent Approval and Resume - #599
lia-by-librechat[bot] wants to merge 13 commits into
Conversation
|
Head c66dc21: foreground graph subagents use regular approval/replay machinery with member attribution and graph-definition binding. Parallel approvals, rebuilt-run recovery, question interrupts, decision handling, and cancellation are covered. All 460 focused tests, SDK typecheck, scoped lint, and build pass. Fresh independent review follows this head. |
|
Head 704f27c: updates the pre-existing graph host-argument rejection assertion to account for optional graph revisions. Production code is unchanged. Host-argument, graph-normalization, and real graph-HITL cases pass, as do typecheck and scoped lint. Fresh CI and exact-head independent review follow. |
|
Head 6e0f551: fixes review R1/P1 by binding canonical registry capabilities, executable map schemas, and direct-tool modes into graph resume identity. Stale programmatic approvals fail closed; unchanged reordered registries resume. All 521 focused tests across 17 suites pass, along with SDK typecheck, scoped lint, and build. Fresh exact-head independent review and CI follow. |
|
@codex review the latest head, final 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: 6e0f5516b3
ℹ️ 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".
| members: config.agents.map((agent) => ({ | ||
| id: agent.agentId, | ||
| provider: agent.provider, | ||
| model: resolveClientOptionsModel(agent.clientOptions), | ||
| instructions: agent.instructions, | ||
| additionalInstructions: agent.additional_instructions, | ||
| maxContextTokens: agent.maxContextTokens, |
There was a problem hiding this comment.
toolEnd in the graph resume fingerprint
When a pending graph approval is resumed after a rebuild where a member's toolEnd setting changed, this member projection produces the same definition ID, so isResumeExecutionCompatible accepts the new graph. However, Graph.ts uses agentContext.toolEnd to compile the tool-node edge to either END or the agent node, meaning the approved call can resume into a different control flow and terminate the member prematurely instead of returning to the model. Add toolEnd to the fingerprint so this configuration change fails closed like the model, instructions, topology, and tool-schema changes already covered here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. Already covered in a55b691: the member fingerprint includes normalized toolEnd, and the real graph-HITL regression rejects a changed tool-end setting before approved execution. No additional patch is needed.
|
Head a55b691: fixes R2/P1 by binding declared and effective member session partitions across rebuilt and in-memory resume, including context-adapter removal. The invariant sweep also binds tool-end and summarize-only controls. Default legacy session keys remain compatible. All 599 focused tests across 19 suites pass, with SDK typecheck, PR-touched-file lint, and build. R1/P1 remains fixed in 6e0f551. Fresh exact-head review and CI follow. |
|
Head e4adb46: fixes R4/P2 by restoring checkpoint evidence for each resumed member independently. Simultaneous approvals, mixed decisions, partial resume, and a follow-up question pass without replaying completed siblings. Existing owner, proposal, principal, and conversation checks remain intact. All 662 focused tests across 21 suites pass, with SDK typecheck, all touched-file lint, and package build. Fresh exact-head independent review and CI follow. |
|
Head 48c821f: fixes R5/P1 by restoring settled records for every pending member while granting approval evidence only to addressed interrupts. Also fixes R6/P2: checkpointed pending IDs let Run.resume recognize a non-current member’s interrupt map without extra reads. The new real SDK tests cover either partial order, both decisions, fresh/in-memory resume, and completed direct effects beside direct/event approvals. Boundary checks, typecheck, and lint passed; final focused checks/build are running alongside CI. Fresh exact-head independent review follows. |
|
Independent review completed at head 48c821f: no findings. R1–R6 remain fixed or already covered; no rejected findings. All 683 focused tests, SDK typecheck, touched-file lint, and build passed. CI: 12 passed; Anthropic summarization is running. Reviewer verification was source-only. Full local suite, live-provider tests, and real Langfuse inspection were not run. No publication or LibreChat dependency/UI change. |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48c821fc3e
ℹ️ 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".
| maxTurns: config.maxTurns, | ||
| entry: config.entryAgentId, | ||
| result: config.resultAgentId, | ||
| edges: config.edges, |
There was a problem hiding this comment.
When a graph uses a function-valued edge.prompt and a rebuilt run supplies a different callback without changing the optional configId, both definitions receive the same fingerprint: stableStringify ultimately applies JSON.stringify to the function, which yields undefined. The pending approval is therefore accepted even though the resumed graph can invoke different prompt logic after executing the approved tools. Require a stable revision for functional prompts or otherwise incorporate their executable identity into the compatibility check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cc511a. Foreground HITL graphs with function-valued edge prompts require a host configId revision before child execution. The fingerprint distinguishes callback and static prompts. Tests cover changed revisions, unchanged rebuilt callbacks, prompt-kind changes, and non-HITL backwards compatibility.
| const member = childPlan.memberInputs.get(agentId); | ||
| if (member == null) throw new Error('Unknown subagent context member.'); | ||
| member.codeSessionKey = sessions.codeSessionKey; | ||
| member.initialSessions = sessions.initialSessions; |
There was a problem hiding this comment.
When prepare() returns different initialSessions on resume while keeping the same codeSessionKey, this assignment accepts the new seed, but its effect depends on whether the child graph remained in memory. seedChildGraphSessions merges newly returned files into a cached paused graph, whereas a rebuilt graph is seeded and then restoreSubagentResumeState clears those sessions and restores the checkpoint snapshot, discarding the same files. Consequently, the identical approval resume can execute a code tool with different file grants depending on whether the process rebuilt the Run; reject changed session seeds or apply refreshed authorization after checkpoint restoration consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3cc511a. Current host-authorized initial session seeds are merged after checkpoint restoration, consistently for cached and rebuilt children. Existing execution sessions and generated files remain authoritative; fresh file refs deduplicate by storage identity. Direct/event tests verify identical injected grants and untouched host maps.
|
Head 3cc511a: fixes Codex R7/P2 (functional prompt revision binding) and R8/P2 (consistent refreshed session grants). All 692 focused tests across 21 suites, SDK typecheck, PR-touched-file lint, and build passed. New exact-head independent review and CI follow. |
|
Independent review completed at 3cc511a: no findings. Codex R7/P2 and R8/P2 are fixed; all earlier findings remain resolved. Local verification: 692 focused tests, SDK typecheck, touched-file lint, and build passed. CI: 9 queued, 4 success. Source-only independent review. Full local suite, live-provider tests, and real Langfuse inspection were not run. |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc511ad44
ℹ️ 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".
| maxContextTokens: agent.maxContextTokens, | ||
| toolDefinitions: agent.toolDefinitions, |
There was a problem hiding this comment.
When a paused graph is rebuilt with only a member's discoveredTools changed, this projection produces the same definition ID, so the resume compatibility check accepts it. AgentContext.fromConfig seeds those names into discoveredToolNames, which controls whether deferred registry tools are bound to subsequent model calls; therefore a resume can continue with newly exposed capabilities instead of failing closed like registry and tool-schema changes. Include a normalized discoveredTools set in the member fingerprint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48454f6. Resume identity includes the normalized discoveredTools set. Changed declarations fail closed; reordered/duplicate declarations remain compatible. Child preparation still scrubs inherited discovery state, preserving context isolation. Focused graph replay coverage passes.
| pendingInterruptIds: [...new Set(activeRun.pendingInterrupts.flatMap( | ||
| (pending) => pending.id == null ? [] : [pending.id] | ||
| ))], |
There was a problem hiding this comment.
When multiple graph members pause in the same superstep, this manifest records only their interrupt IDs. Run still retains only interrupts[0], and getInterrupt() strips the private manifest, so a public caller cannot see the proposal or allowed decisions associated with the remaining IDs and therefore cannot safely construct the supported multi-ID resume map without reading the checkpointer directly, as the new integration tests do. Expose the pending ID/payload pairs through the public interrupt result rather than persisting IDs alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48454f6. getInterrupt().pendingInterrupts exposes detached public interruptId/payload pairs, including member proposals and allowed decisions, without private replay/checkpoint metadata. Graph integration tests now construct simultaneous and partial resume maps using this public result only. Direct/event and fresh-Run recovery, immutable proposals, and malformed manifest cases pass.
|
Head 48454f6 includes current main 253bc8e and fixes Codex R9/P2 and R10/P2. SDK typecheck, touched-file lint, and 738 focused tests across24 suites pass, including merged-main stream/provider regressions. New exact-head independent review and CI follow. Package build is not yet rerun: shared disk is below15GB free. No publication or new version bump. |
|
Independent review completed at 48454f6: no findings. R1–R10 are fixed or already covered; no rejected findings. Updated main remains253bc8e47a01e63d979efc5fd0e030c44649ab12. Local SDK typecheck, scoped lint, and738 focused tests passed. Exact-head CI: 13 success. Review was source-only. Local package build remains blocked below15GB free; full local suite, live-provider tests, and real Langfuse inspection were not run. |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48454f6ea7
ℹ️ 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".
| codeSessionKey: agent.codeSessionKey ?? Constants.EXECUTE_CODE, | ||
| toolEnd: agent.toolEnd ?? false, | ||
| summarizeOnly: agent.summarizeOnly ?? false, | ||
| model: resolveClientOptionsModel(agent.clientOptions), |
There was a problem hiding this comment.
When a paused graph is rebuilt with the same model name but different execution-affecting clientOptions—for example temperature, thinking settings, fallbacks, or API endpoint configuration—this projection produces the same definition ID because it retains only the resolved model. AgentContext.fromConfig passes the full clientOptions object into the resumed member, so the approved tool call can continue under materially different model behavior instead of failing closed. Include normalized runtime options in the fingerprint, or require a revised configId for options that cannot be safely serialized.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61d70d0. Canonical SHA-256 option identity binds serializable model/runtime/endpoint/fallback inputs without persisting raw option values. Executable, cyclic, accessor, and opaque options require a host configId revision before resumable work starts. Changed options fail closed; key-order and omitted undefined values remain compatible. Tests include host revisions and non-executed getters.
| delete configurable[TOOL_BATCH_REPLAY_KEY]; | ||
| delete configurable[TOOL_APPROVAL_REVIEW_CONFIG_KEY]; | ||
| Object.assign(configurable, first); | ||
| if (configs.size > 1) configurable[TOOL_REPLAY_CONFIGS_KEY] = configs; |
There was a problem hiding this comment.
When parallel graph members pause in the same superstep with a tool approval in one branch and an ask_user_question call in another before that branch has any settled replay records, only the approval contributes an owner, so configs.size is one and this omits the per-owner map. Resuming both advertised interrupt IDs then exposes the first interrupt's global replay evidence to both ToolNodes; if the approval is first, the question branch fails the owner check, while the opposite ordering leaves the approval without its checkpointed review evidence. Preserve per-task isolation even when some pending interrupts have no replay owner, rather than falling back to one global evidence object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61d70d0. Ownerless pending interrupts install explicit empty per-execution replay evidence, so neither pending order can reuse another member's approval evidence. Same-execution question replay is isolated; foreign principal/thread identities still fail closed. Tests cover both orderings, partial/simultaneous decisions, and real fresh/in-memory mixed approval/question runs.
|
Head 61d70d0 includes latest main f4695e4 and fixes Codex R11/P2 and R12/P2. All892 focused tests across24 suites, typecheck, PR-scoped lint, package build, and16 light-entry checks pass. One additional unchanged main cap-test fails identically on frozen main with this worker's long temp path; no unrelated patch. Fresh exact-head review and CI follow. No publication or PR merge. |
|
Head 61d70d0: current main is included and fresh Codex R11/P2 and R12/P2 are fixed, with replies on both threads. Local892 focused tests, typecheck, touched-file lint, build and16 light-entry checks pass. CI: 13 success. Independent review is incomplete: frozen SDK object unavailable ( |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d70d06fb
ℹ️ 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".
| members: config.agents.map((agent) => ({ | ||
| id: agent.agentId, | ||
| provider: agent.provider, | ||
| codeSessionKey: agent.codeSessionKey ?? Constants.EXECUTE_CODE, | ||
| toolEnd: agent.toolEnd ?? false, |
There was a problem hiding this comment.
When a paused graph is rebuilt after changing an omitted member option such as maxToolResultChars, summarizationEnabled, summarizationConfig, or contextPruningConfig, this projection produces the same definition ID and accepts the resume. The fresh AgentContext consumes those settings, so the approved tool result can be truncated differently or subsequent model control flow can change; include all execution-affecting member configuration in the resume identity, excluding only settings intentionally refreshed during resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9aa4d11. Stable member configuration is now hashed by default, including summarization, pruning, truncation, content and streaming settings. Only refreshed sessions, scrubbed parent state and observational/cache inputs are excluded; normalized tool declarations remain separately bound. Tests reject each changed execution setting before effects.
| pendingInterrupts: [...pendingInterrupts.values()].map((pending) => ({ | ||
| interruptId: pending.interruptId, | ||
| payload: structuredClone(getPublicToolInterruptPayload(pending.payload)), |
There was a problem hiding this comment.
When a custom graph node interrupts with a non-structured-cloneable payload, such as one containing a function or symbol, getInterrupt() now throws here instead of returning the custom payload; structured cloning can also erase a custom class prototype. The primary payload path intentionally preserves non-tool-approval payloads, and the public generic documents arbitrary custom interrupt payloads, so pendingInterrupts should use the same preservation semantics rather than cloning every value unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9aa4d11. Pending proposals use the same approval-only cloning semantics as the primary payload. Custom functions, symbols and class instances are preserved rather than structured-cloned. Public result tests cover all three and primitives; existing approval isolation coverage remains passing.
| toolMap: Object.fromEntries( | ||
| Array.from(agent.toolMap ?? [], ([key, tool]) => [ | ||
| key, | ||
| convertToOpenAITool(tool), | ||
| ]) | ||
| ), | ||
| tools: agent.tools?.map( | ||
| (tool) => (isLangChainTool(tool) ? convertToOpenAITool(tool) : tool) | ||
| ), | ||
| graphTools: agent.graphTools?.map((tool) => convertToOpenAITool(tool)), |
There was a problem hiding this comment.
When a paused graph is rebuilt with a different implementation for a direct tool in toolMap or graphTools but the same name, description, and schema, convertToOpenAITool produces the same fingerprint and the resume is accepted. The approved call then invokes the replacement function, which can perform materially different side effects despite an unchanged proposal; executable member tools should require a configId revision, as functional edge prompts and opaque model options already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9aa4d11. Foreground graph HITL requires a host configId revision for executable tools, toolMap and graphTools before any member runs. Changed implementation revisions reject saved approvals; schema-only event tools remain supported without a revision. Tests cover all executable sources and fresh rebuilt resume.
|
Head 9aa4d11 addresses all three P2 findings from the final Codex review requested in comment6343834691. All910 focused tests across25 suites, typecheck, touched-file lint/import order, package build and16 entry checks passed. New exact-head CI and independent review follow. No publication or PR merge. |
|
Head ce0c9bb completes R13's execution-setting audit by including cached schema-token budgets. All911 focused tests/25 suites, typecheck, scoped lint, package build and16 light-entry checks pass. R13–R15 remain resolved with replies. CI restarts for this exact head. Independent review is capacity-blocked (503COMMAND_UNAVAILABLE), not clean. Monitoring found no newer Codex comments after the completed requested review. No merge or publication. |
|
Head ce0c9bb: all3 P2 findings from the requested final Codex review are fixed and replied to. Local911 focused tests, types, lint/imports, build and16 entry checks pass. CI: 13 success. The requested review completed; another12-minute watch saw no new Codex feedback. Independent review remains capacity-blocked, with no clean-current-head claim. No merge or publication. |
This branch has not been deployed
2 participants
Summary
Enable foreground graph-subagent HITL through existing approval/checkpoint/replay contracts. Members resume separately or together, including after rebuilding
Run, without repeating completed effects. Recovery retains the designated result.Public pending ID/proposal pairs support multi-member review without private checkpointer access. Tool approvals are detached; custom interrupt payload semantics remain unchanged.
Resume binds stable member configuration by default, normalized tools/discovery, topology, and session partitions. Executable tools, functional prompts and opaque member options require a host
configIdrevision. Refreshed authorized sessions merge after restoration. Mixed question/approval replay remains member-isolated and identity-bound.Exports
GRAPH_SUBAGENT_HITL_VERSION = 1. Graph nesting remains disabled; background work cannot wait for input. Hosts retain authorization and single-winner resume ownership. No publication, extra version bump, or LibreChat upgrade. Related: LibreChat-AI/LibreChat#16764.Verification
Head:
ce0c9bb3eca9fb6780247f59242d4b0c20dd1405. Includes mainf4695e4a715632b76a5f77f867c95dfe3b880e12.tsc --noEmit, touched-file lint/import order, package build, and16 light-entry checks passed. Feature export verified.503 COMMAND_UNAVAILABLE); no clean review is claimed.Finding Ledger
R1–R12 remain addressed: registry/runtime options, member partitions, tool-end controls, scoped replay, settled siblings, partial maps, prompt revisions, refreshed sessions, discovered tools, public pairs and ownerless question isolation.
9aa4d113.9aa4d113.9aa4d113.No rejected findings. Legacy sessions/manifests remain readable. Older custom-partition snapshots without identity fail closed.
Current Review And CI
Head
ce0c9bb3eca9fb6780247f59242d4b0c20dd1405includes all3 P2 fixes from Codex review5616321422. Replies identify the resolving commits. Stable execution binding also includes cached schema-token budgets.503 COMMAND_UNAVAILABLE); no clean review claimed.61d70d06completed with3 findings, now fixed. Follow-up monitoring15:12–15:25UTC found no newer feedback. That review does not cover this replacement head.