refactor(agent-core-v2): absorb the human layer and replace the agent loop with a facade over its turn state machine - #3580
Conversation
|
commit: |
![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: 5e38e379cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| progressHandlers.set(input.toolCall.id, input.onUpdate); | ||
| try { | ||
| let matched: ToolExecutionResult | undefined; | ||
| for await (const result of options.toolExecutor.execute([input.toolCall], { |
There was a problem hiding this comment.
When one model response contains AgentSwarm alongside another tool, or contains multiple AgentSwarm calls, invoking the executor separately with [input.toolCall] makes every BeforeToolExecuteEvent.toolCalls value a singleton. The guard in features/swarm/agent/swarmService.ts therefore cannot detect the forbidden mixed or repeated swarm batch and allows all calls to launch; retain the original response batch when adjudicating each call.
Useful? React with 👍 / 👎.
| if ( | ||
| result.isError === true && | ||
| (result.stopBatchAfterThis === true || result.stopTurn === true) | ||
| ) { |
There was a problem hiding this comment.
When a successful tool returns stopBatchAfterThis: true or stopTurn: true, this isError condition leaves batchStopped false, so the serialized calls queued after it still execute. For example, successful UpdateGoal completion/blocking and over-budget SetGoalBudget results carry these stop signals specifically to prevent later calls from running, so a model response containing one of them followed by another side-effecting tool violates the requested stop.
Useful? React with 👍 / 👎.
…name vendored kosong to llm-adapter
…e human turn state machine
5e38e37 to
ba05a69
Compare
sailist
changed the title
…ne tools adapter - delegate each step's tool calls to the v2 tool executor as one whole batch: beginBatch arms the expected call set from the llm.done entry, per-actor execute calls collect into a pending barrier, and the full batch is submitted in tool-call order once complete - drop the serial batchChain/batchStopped/skipped approximation so the executor's prepare-stage stopBatchAfterThis skip, veto short-circuit, and ToolScheduler access-based concurrency apply natively again
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f19e2551b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (job === undefined || job.turn.state !== 'queued') return false; | ||
| const reservation = this.reservations.find((entry) => entry.id === turnId); | ||
| if (reservation === undefined || reservation.cancelled) return false; | ||
| reservation.cancelled = true; |
There was a problem hiding this comment.
When a turn is submitted while an idle quiescence lease is held and then canceled before the lease is released, this only marks the unlaunched reservation as canceled and leaves it at the head of reservations. A later submission launches its machine prompt, but bindMachineTurn() consumes that stale canceled reservation, suppresses the machine turn, and leaves the new reservation permanently marked as launched with no prompt queued, so the subsequent turn never starts or settles. Remove an unlaunched reservation when canceling it, while retaining launched placeholders needed to align with the machine queue.
Useful? React with 👍 / 👎.
| export function createMachineTools(options: CreateMachineToolsOptions): MachineTools { | ||
| const extras = new Map<string, ToolResultExtras>(); | ||
| const progressHandlers = new Map<string, ((update: ToolUpdate) => void) | undefined>(); | ||
| const knownNames = new Set(options.toolInfos.map((info) => info.name)); |
There was a problem hiding this comment.
Fresh evidence after the earlier batch fix is that knownNames is still captured once when the engine is first created, while MCP reconnect/discovery and user/runtime tool activation can register tools later. Calls to those tools are consequently treated as unknown by the machine and excluded from expectedIds; the fallback in executeUnknownToolCall() then invokes the executor separately, so a response mixing a newly registered tool with AgentSwarm again hides the full batch from the swarm guard and can launch a forbidden mixed batch. Build the adapter from the current registry for each batch or otherwise update its definitions when registrations change.
Useful? React with 👍 / 👎.
…r path and backfill aborted tool results on forced abort
1 participant
Related Issue
N/A — internal architecture refactor, no user-facing behavior change intended.
Problem
The
humanexecution layer (hand-written xstate turn/agent machines and thellmlayer) graduated as an architecture experiment but lived outside production agent-core-v2, which ran its own step-request Loop — two parallel execution models for the same agent runtime. This PR absorbs the layer into v2 assrc/human/and replaces the v2 loop's execution core with its turn state machine, so a single execution model remains.What changed
Absorb the human layer into v2 (
src/human)packages/agent-core-v2/src/human/; its standalone package is deleted.src/kosongis mostly deleted; the parts still in use are renamed tosrc/llm-adapter/.src/llm-adapterand the newsrc/agent/loop/machineare the only adapter scopes allowed past the human vocabulary modules).namepreservation from feat(protocol): preserve media attachment names #3548 is carried into the human message contract (name?: stringon image/video URL parts), so its wire/projection behavior does not regress.llm-adapternow come from#human/llm/*instead of being duplicated.Agent loop replaced by the human turn state machine (facade, no queue state)
src/agent/loopno longer holds queue state: the StepRequest/admission system (stepRequest,stepRequestQueue,loopContinuation,handoffStep, prompt step requests, and the stepRetry engine) is deleted. Turn queueing, mid-turn steer, notifications, and reminders run entirely on the agent machine's internal state.IAgentLoopServicebecomes a facade with intent-style methods (submit/steer/notify/cancel/ status / quiescence) mapped onto machine events and snapshots; consumers (prompt, task, goal, external hooks, tool dedupe, full compaction) migrate to it.src/agent/loop/machine/bridges the frozensrc/human/machines to v2 services: the layer'sLlmRequesteroverIAgentLLMRequesterService(event bridging, error-kind mapping for machine retry decisions), tool definitions overIAgentToolExecutorService(permission/telemetry/truncation path unchanged), and an engine that normalizes machine emissions into step-granular events.turnKeystate incl. cancelled-queued turns), telemetry, hooks (onWillBeginStep/onDidFinishStep),TurnStepRetrying, andfilterederror mapping. kap-server / klient / TUI see no protocol change.LoopErrorHandlerbecause machine recovery proposals are synchronous and cannot host the async compaction flow.resolve(model)(it previously ducked the interface via casts), matching the requester-based model resolution.Verification
test/agent/loop65/65,test/agent1858/1858,test/features949/949, acp-server 149/149, klient 129/129 green with zero net test-count change.pnpm typecheckexit 0;check-no-comments,check-import-boundaries, andoxlint --type-awareclean.Checklist
/approve). — N/A, internal refactor.gen-changesetsskill, or this PR needs no changeset. — No changeset: internal refactor with no user-perceivable change.gen-docsskill, or this PR needs no doc update. — No doc update.