Fix Hermes session restore: treat per-turn session-end as a turn boundary#5009
Merged
Conversation
…e record Hermes fires on_session_end once per conversation turn, not at the true session boundary. cmux maps it to the destructive session-end action, so after the first turn the restore record and surface resume binding are consumed and nothing survives a quit/relaunch. This test asserts the per-turn session-end routes through the non-destructive turn-boundary path (recordPromptStop) and does not call store.consume / clearAgentSurfaceResumeBinding. It fails on current code. Refs #5000 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hermes fires the on_session_end plugin hook once per conversation turn (end of every run_conversation()), not at the true session boundary. cmux maps both on_session_end and on_session_finalize to the destructive session-end action, and the .sessionEnd handler only spared grok and antigravity from consuming the session via a hardcoded name check. Because hermes-agent was not in that list, the restore record and surface resume binding were destroyed after the first turn, so a quit/relaunch had nothing to restore. Replace the hardcoded name check with a declared sessionEndIsTurnBoundary property on AgentHookDef (alongside publishesStopNotification), set true for grok, antigravity, and hermes-agent. This turns an implicit, drift-prone name registry into a typed invariant on the definition and fixes the entire class of 'per-turn session-end destroys the restore record' bugs for any restorable agent that opts in. grok and antigravity behavior is unchanged. Fixes #5000 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds AgentHookDef.sessionEndIsTurnBoundary and AgentHookAction.sessionFinalize, sets the flag for grok/antigravity/hermes-agent, updates cmux to use the flag and a unified teardown helper, and adds a Hermes integration test distinguishing per-turn session-end from session-finalize teardown. ChangesSession-End Turn Boundary Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR fixes the Hermes session restore regression (#5000) by correctly separating the per-turn
Confidence Score: 5/5Safe to merge. The fix is minimal and correct: per-turn session-end is non-destructive, and true teardown only fires on session-finalize. The key concern from the prior review iteration — both on_session_end and on_session_finalize routing to the same session-end subcommand — is fully addressed. on_session_finalize now routes to the new session-finalize subcommand and .sessionFinalize action, which calls performAgentSessionTeardown() unconditionally. The turn-boundary flag only guards the .sessionEnd case. grok and antigravity behavior is structurally unchanged. The integration test covers both the non-destructive path and the destructive teardown path. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca4a712. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CLIGenericHookPersistenceTests.swift`:
- Around line 682-805: The new test
testHermesAgentSessionEndIsTurnBoundaryAndPreservesRestoreRecord in
CLIGenericHookPersistenceTests.swift uses XCTest but project guidelines require
Swift Testing; to avoid a large migration, leave this test as-is for consistency
with the existing XCTest-based suite, and create a tracked follow-up (create an
issue/PR) to migrate CLINotifyProcessIntegrationRegressionTests and related
XCTest suites to Swift Testing; add a brief TODO comment above
testHermesAgentSessionEndIsTurnBoundaryAndPreservesRestoreRecord referencing
that issue so reviewers know the deviation is intentional and trackable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 366211cd-6d8b-4399-b2e3-b2f7c395d304
📒 Files selected for processing (3)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/cmux.swiftcmuxTests/CLIGenericHookPersistenceTests.swift
…n action Cursor Bugbot correctly flagged that hermes maps both on_session_end (per-turn) and on_session_finalize (true teardown) to the session-end subcommand, so the sessionEndIsTurnBoundary flag made the genuine teardown non-destructive too — leaking the restore record, PID routing, and resume binding indefinitely. Route on_session_finalize to a new session-finalize subcommand backed by a new AgentHookAction.sessionFinalize that owns the destructive cleanup. The per-turn session-end still takes the non-destructive turn-boundary path; a non-turn-boundary session-end (gemini/copilot/etc.) falls through to the shared teardown, so their behavior is unchanged. Only hermes routes to the new action. Extend the regression test to also assert session-finalize consumes the record, clears the resume binding, and clears agent PID routing. Refs #5000 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 28067-28070: Extract the shared destructive cleanup that currently
uses `fallthrough` into a helper function (e.g.,
`performSessionFinalizationCleanup`) and call it from both the non-turn-boundary
teardown case and the `.sessionFinalize` case instead of falling through; the
helper should accept the `sessionId`, `store: AgentHookSessionStore`, and `env:
[String: String]`, perform the `store.lookup(sessionId:)` guard, call
`sendAgentFeedTelemetry(mapped.workspaceId)`, compute
`shouldSuppressNestedAgentVisibleMutations(currentAgentPID:mapped.pid, env:
env)`, and contain the rest of the destructive cleanup logic, then replace the
`fallthrough` with an explicit call to that helper from the original case and
keep `.sessionFinalize` calling the same helper so the logic is explicit and
SwiftLint warnings are removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d030ab05-b9d0-41c8-bede-d3c10ff01ca7
📒 Files selected for processing (3)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/cmux.swiftcmuxTests/CLIGenericHookPersistenceTests.swift
Addresses CodeRabbit review: replace the fallthrough from .sessionEnd into .sessionFinalize with an explicit, documented performAgentSessionTeardown() nested helper called from both the non-turn-boundary session-end path and the dedicated .sessionFinalize action. Behavior is unchanged; this removes the SwiftLint 'fallthrough should be avoided' warning and makes the shared destructive cleanup explicit. Refs #5000 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Hermes loses its restore record after the first conversation turn, so quitting and relaunching cmux with a live Hermes session restores nothing.
Root cause. Hermes fires the
on_session_endplugin hook once per conversation turn (at the end of everyrun_conversation()), not at the session boundary. The true session boundary is the separateon_session_finalizehook. InCLI/CMUXCLI+AgentHookDefinitions.swiftbothon_session_endandon_session_finalizemap to the samesession-endsubcommand →.sessionEndaction.In
CLI/cmux.swift's.sessionEndhandler, a non-destructive turn-boundary path already existed (store.recordPromptStop(...)), but it was gated by a hardcodeddef.name == "grok" || def.name == "antigravity"check.hermes-agentfell through to the destructive branch —store.consume(...)(which doesstate.sessions.removeValue(forKey:)) plusclearAgentSurfaceResumeBinding(...)— destroying the restore record and resume binding after the first turn.Fix
Replace the hardcoded name check with a declared
sessionEndIsTurnBoundary: Boolproperty onAgentHookDef(alongside the existingpublishesStopNotification/createConfigDirIfMissingflags), settrueforgrok,antigravity, andhermes-agent. The.sessionEndhandler now branches ondef.sessionEndIsTurnBoundary.This turns an implicit, drift-prone name registry into a typed invariant on the definition (per the repo's "no parallel hand-maintained registries" discipline) and fixes the entire class of "per-turn session-end destroys the restore record" bugs for any restorable agent that opts in — not just hermes. grok and antigravity behavior is byte-for-byte unchanged (they now read the flag instead of their name).
Structural follow-up (not in this PR)
The deeper structural option is to split the shared mapping so
on_session_endalways routes through the turn-boundary path and onlyon_session_finalizetriggers destruction. That requires a new subcommand string + action and re-installing the hook contract in users'~/.hermes/config.yaml, with a regression risk ifon_session_finalizedoesn't fire reliably — a larger change than is safe to land here. The declared-flag fix is the minimal, correct, low-risk version and matches how grok/antigravity already behave (their restore records also persist across the session-end hook). Left as a follow-up.Test
Two-commit red/green structure:
testHermesAgentSessionEndIsTurnBoundaryAndPreservesRestoreRecordincmuxTests/CLIGenericHookPersistenceTests.swift, extending the existing grok/antigravity keep-list suite. It drives the real CLI (hooks hermes-agent session-start→agent-response→session-end) against a mock socket and asserts the per-turnsession-end:clear_agent_pid hermes-agent.…,surface.resume.clear,hermes-agent-hook-sessions.json(not consumed).Reproduction note: the user-visible bug surfaces only across a quit/relaunch with a multi-turn live Hermes session, which is not practical to script locally. The unit test pins the exact handler behavior (turn-boundary path, no consume/clear) that the relaunch restore depends on.
Fixes #5000
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes hook routing and session store lifecycle for Hermes and restorable agents; incorrect finalize firing could leave stale state or skip cleanup, but behavior for grok/antigravity is intended to be equivalent and covered by new tests.
Overview
Fixes Hermes (and similar agents) losing session restore after the first turn by treating per-turn
session-endas a turn boundary instead of full teardown.Adds
sessionEndIsTurnBoundaryonAgentHookDef(enabled for grok, antigravity, hermes-agent) so.sessionEndusesrecordPromptStopand does not consume the hook session or clear resume/PID routing. Maps Hermeson_session_finalizeto a newsession-finalizesubcommand andperformAgentSessionTeardown()for real cleanup (consume store,surface.resume.clear,clear_agent_pid). Replaces the hardcodedgrok/antigravityname check in the handler.Adds integration test
testHermesAgentSessionEndIsTurnBoundaryButFinalizeTearsDowncovering per-turn preservation vs finalize teardown.Reviewed by Cursor Bugbot for commit 7c82253. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fix Hermes session restore by treating per-turn on_session_end as a turn boundary and routing on_session_finalize to a dedicated teardown. Sessions now persist across quit/relaunch and finalize cleans up correctly. Fixes #5000.
sessionEndIsTurnBoundarytoAgentHookDef; enabled forgrok,antigravity,hermes-agent.session-finalizesubcommand andAgentHookAction.sessionFinalize; mapped Hermeson_session_finalizethere.performAgentSessionTeardown()for non–turn-boundary.sessionEndand.sessionFinalize.testHermesAgentSessionEndIsTurnBoundaryButFinalizeTearsDownto verify per-turn preservation and teardown on finalize.Written for commit 7c82253. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests