fix(acp): steer Hermes without replacing active turns - #4154
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: 09fe435249
ℹ️ 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 (this.concurrentSteerAdmissionCount > 0) { | ||
| this.deferredForegroundSettlement = this.promptResponseEvent(response, turnId); | ||
| return; |
There was a problem hiding this comment.
When the user presses Stop and the original ACP prompt resolves as cancelled while the concurrent steering prompt remains unresolved, this branch defers the cancellation forever because concurrentSteerAdmissionCount never reaches zero. AgentManager eventually force-cancels only its own run bookkeeping after a timeout, while this session retains activeForegroundTurnId; the next prompt then fails with “A foreground turn is already active.” Process canceled/failed responses immediately, or explicitly invalidate outstanding steer admissions during interrupt(), so a hung auxiliary prompt cannot leave the Hermes session unusable.
Useful? React with 👍 / 👎.
Let Generic ACP providers opt into concurrent session/prompt steering and enable that transport for Hermes. Preserve foreground ownership across response ordering, permission release, exact echo identity, Stop preemption, and stale steer completion.
wrapSessionProvider forwards a fixed list of the optional AgentSession methods and steerActiveTurn was not on it, so any wrapped session lost the method entirely. steerOrReplaceActiveTurn then took its unavailable branch and replaced the running turn instead of steering it. That hits every custom provider, and a built-in provider as soon as profileModels or additionalModels is configured. The claude, codex and opencode providers all implement steerActiveTurn, so with Default send set to Steer a mid-turn message canceled the turn and the tool call in flight came back denied. The wrap test already walks the optional methods, so steerActiveTurn joins that walk and the exhaustiveness list next to it.
Cover both concurrent prompt response orders, permission release, Stop preemption, stale completion isolation, real Hermes steering, and redacted evidence generation.
9d33420 to
0b00d2c
Compare
|
cc @boudra This is the Hermes ACP counterpart to #3752 and is ready for independent review/QA at Highlights:
Local evidence on the rewritten exact SHA:
The PR body also records two non-blocking follow-ups found during isolated deployment: the CLI does not yet expose an explicit Could you take a look and run the normal independent QA/delivery flow when appropriate? Fork Actions are currently waiting for maintainer approval rather than reporting code failures. |
|
| Filename | Overview |
|---|---|
| packages/server/src/server/agent/providers/acp-agent.ts | Adds concurrent ACP steering, exact echo suppression, permission clearing, deferred terminal settlement, and stale-response generation guards. |
| packages/server/src/server/agent/providers/hermes-acp-agent.ts | Introduces the Hermes-specific generic ACP client that defaults concurrent-prompt steering while preserving explicit opt-out configuration. |
| packages/server/src/server/agent/provider-registry.ts | Preserves optional steering through session wrappers and selects the Hermes client for canonical derived Hermes providers. |
| packages/server/src/server/agent/providers/acp-agent.test.ts | Covers steering lifecycle and identity scenarios extensively, but several cases are coupled to private lifecycle state and mocked call details. |
| packages/server/src/server/daemon-e2e/acp-active-turn-dispatch.e2e.test.ts | Exercises response ordering, permission release, and explicit Stop through a daemon fixture, with some assertions coupled to raw ACP framing. |
| packages/app/src/data/acp-provider-catalog.ts | Enables concurrent-prompt steering in newly generated canonical Hermes provider configurations. |
Sequence Diagram
sequenceDiagram
participant U as Client
participant M as AgentManager
participant A as ACP session
participant H as Hermes
U->>M: steer active turn
M->>A: steerActiveTurn(expectedTurnId)
A->>A: validate input and clear permissions
A->>H: concurrent session/prompt
A-->>M: accepted
H-->>A: original prompt response
A->>A: defer foreground settlement while steer is pending
H-->>A: steer prompt response
A->>A: decrement admission count
A-->>M: emit one foreground terminal event
Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/hermes-acp-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test("does not send when clearing a pending permission fails", async () => { | ||
| const session = createSession(); | ||
| const { prompt, cancel } = primeConcurrentSteer(session); | ||
| vi.spyOn(session, "respondToPermission").mockRejectedValue(new Error("deny failed")); |
There was a problem hiding this comment.
The new steering tests replace session methods, inspect mock call storage, mutate private lifecycle fields, and assert raw ACP framing rather than exercising the caller-facing session and daemon interfaces. This couples the suite to implementation details, so internal refactors can break tests while defects in the real composition remain hidden.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3 participants
Linked issue
Addresses #3964 and #3999. Related to #4061 and #4112.
This PR carries the exact patch from #4113 as a dedicated commit with Burak Cabadan's original authorship, because derived ACP providers otherwise lose
steerActiveTurnat the wrapper boundary.Type of change
Reasoning
Paseo currently treats an unavailable ACP steer as interrupt-and-replace. That is particularly destructive with Hermes: a message sent while Hermes is running a tool cancels the tool, starts another foreground turn, and can strand Hermes' own queued prompt behind its interrupted ACP turn.
Hermes already has the transport needed for the intended behavior. A second plain-text
session/promptreceived while its turn is active is redirected into the sameAIAgentturn. This change lets a generic ACP provider explicitly opt into that transport. A small internalHermesACPAgentClientowns the Hermes default, following the existing provider-specific ACP client pattern instead of adding command sniffing to the generic registry. Paseo no longer sendssession/cancelfor an ordinary Hermes steer.This follows the optional
AgentSession.steerActiveTurncontract used by the Pi implementation in #3752 while preserving provider-specific acknowledgment semantics. Pi can queue first because its nativesteerRPC acknowledges admission; ACPsession/promptresolves only when the turn ends, so concurrent ACP steering clears a blocking permission before dispatch and sends nothing if that clear fails.Goals
session/prompt, without canceling or replacing its foreground turn.hermes acpprovider configs while preservingactiveTurnSteering: "none"as an explicit opt-out.Non-goals
_session/steeringextension support; Add steering support for ACP providers that indicate support #3921's vendor-extension approach was reverted and is separate from Hermes' existing redirect transport./reload-mcpor broader Hermes CLI/ACP command parity work.QA
Before
On Paseo 0.7.0 with the installed Hermes ACP provider, I started a foreground
sleep 60tool call and sent a message while it was active. Paseo canceled the original turn and sent a replacement prompt. The observed result was:The replacement produced a second Paseo foreground turn and the queued Hermes message did not run.
A deterministic ACP subprocess fixture also recorded one
session/cancelfollowed by the replacementsession/prompton the unmodified path.After
Tested on macOS 26.2, Apple Silicon, Paseo 0.7.0, and:
The final exact-SHA test bundle used an in-process daemon on an OS-assigned loopback port with a temporary
PASEO_HOMEand temporary workspace. It did not restart or send test traffic through the normal daemon on port 6767.Focused deterministic daemon E2E:
Those cases cover:
session/cancelpromptly, emit one canceled turn, and never resume from the late steer response.Real Hermes E2E on exact candidate
0b00d2c59f60e65a958faeffeb3f4d1aa6c14bb8:Observed redacted summary:
{ "candidateHead": "0b00d2c59f60e65a958faeffeb3f4d1aa6c14bb8", "actual": { "finalStatus": "idle", "turnStartedCount": 1, "turnCanceledCount": 0, "assistantText": "Redirected the active turn with your correction. HERMES_ACP_STEER_ACK" }, "nextPrompt": { "status": "idle", "acknowledged": true }, "cleanup": { "workspaceRemoved": true, "paseoHomeRemoved": true } }The completed shell tool contained both
HERMES_ACP_STEER_STARTandHERMES_ACP_STEER_END. The initial prompt and correction share<turn-1>; the health-check prompt is<turn-2>. The evidence artifacts contain no raw UUIDs or local home paths, and both temporary roots were removed.Focused adapter, registry, catalog, deterministic daemon, and evidence tests on the final candidate:
An additional lifecycle regression holds a steer from turn A, interrupts A, admits and defers a steer on turn B, then proves the stale A completion cannot decrement or flush B's settlement state.
Repository gates:
Not tested manually on Windows, Linux, iOS, Android, or Electron UI. This change has no UI surface.
Known follow-ups from isolated local deployment
Before the history cleanup, I built the equivalent code tree at
9d334208a26990211824c91bd9f0b891ee3ec747and ran it as a second daemon with an isolatedPASEO_HOMEon127.0.0.1:6769, while the normal daemon remained on6767. The installed Hermes provider completed a 60-second tool, accepted a correction throughactiveTurnBehavior: "steer"on the same turn without cancellation or replacement, and then completed a normal next turn. The rewritten candidate above was revalidated only through local unit, deterministic daemon, and ephemeral real-Hermes tests; it was not redeployed.That deployment exposed two independent follow-ups which are intentionally outside this PR:
paseo sendCLI currently calls ordinarysendAgentMessagewithout settingactiveTurnBehavior: "steer". A CLI message sent during an active turn is therefore queued for the next turn, while the UI/composer and direct client API can exercise same-turn steering. A future CLI change could expose this explicitly, for example aspaseo send --steer.process waittimeline row can remainrunningafter the process and agent have completed. The same stale status reproduces without any steering message, so it is an existing Hermes ACP tool-status mapping issue rather than a regression from concurrent steering.Checklist
npm run typecheckpassesnpm run lintpassesnpm run formatpasses