Repository navigation
⏯️ feat: Enable Graph Subagent Approval and Resume - #599
lia-by-librechat[bot] wants to merge 9 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. |
This branch has not been deployed
2 participants
Summary
Enable foreground graph-subagent HITL through the existing approval/checkpoint pipeline. Member approvals resume individually or together, including after rebuilding
Run, without repeating completed effects. Recovery retains the designated result.getInterrupt().pendingInterruptsexposes detached public ID/payload pairs for every pending member proposal. Hosts can inspect proposals and allowed decisions and build resume maps without checkpointer access. Private replay metadata stays private.Resume binds graph declarations, tool capabilities/discovery, member session partitions, termination controls, and optional host
configId. Functional edge prompts require a host revision when HITL is enabled. Refreshed host-authorized session seeds merge after restoration.Exports
GRAPH_SUBAGENT_HITL_VERSION = 1. Graph nesting remains disabled; background work cannot wait for human input. Hosts retain authorization and single-winner resume ownership. No publication or LibreChat dependency/UI change.Related: LibreChat-AI/LibreChat#16764.
Verification
Head:
48454f6ea7a014af7af2e91c517cf97f57dd9255. Updated from main253bc8e47a01e63d979efc5fd0e030c44649ab12.tsc --noEmitand PR-touched-file ESLint with zero warnings passed.48454f6ea7a014af7af2e91c517cf97f57dd9255with no findings; R1–R10 rechecked. Reviewer tests/typechecks were not run.Finding Ledger
R1–R8 fixed before this update: registry binding, member session partitions, tool-end controls, batched approval evidence, settled sibling effects, non-current maps, functional prompt revisions, and refreshed session grants.
48454f6e.48454f6e.No rejected findings. Default-session and older manifest reading remain compatible. Older custom-partition snapshots without identity fail closed.