Hermes plugin: stable project scoping + close sessions on process exit#891
Open
zuevrs wants to merge 2 commits into
Open
Hermes plugin: stable project scoping + close sessions on process exit#891zuevrs wants to merge 2 commits into
zuevrs wants to merge 2 commits into
Conversation
added 2 commits
June 10, 2026 22:49
The Hermes plugin used the session cwd as the agentmemory project id. A cwd path is not a stable identity for a repo: ephemeral checkouts (git worktrees, CI runners, agent-orchestrator workdirs) put the same repo under throwaway paths, and unrelated repos often share a checkout basename like 'workdir' - so one repo's memory fragments across paths while different repos cross-bleed into one shared bucket. Resolve the project id as: AGENTMEMORY_PROJECT_NAME env override -> git origin remote repo name (of cwd or a direct child checkout) -> git toplevel basename -> cwd basename (previous behavior). Send the real cwd separately, and pass the resolved project explicitly on the search / remember / smart-search tool calls so recall and save stay inside the project bucket. Signed-off-by: Roman Zuev <[email protected]>
Hermes' cli and gateway paths call on_session_end()/shutdown(), but the ACP adapter path (headless editors and daemons) tears the process down without either - every such run left its agentmemory session dangling as 'active' on the server. Register an atexit hook at initialize() and route all teardown paths through an idempotent _close_session(), so the session is ended exactly once on any graceful exit regardless of which host path ran. Hard kills remain the server-side idle sweep's job. Signed-off-by: Roman Zuev <[email protected]>
|
Someone is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThe Hermes plugin now derives stable project identity from git remotes and working directory, captures cwd alongside project during session initialization, registers graceful atexit shutdown, includes project in memory tool requests, and centralizes session teardown through an idempotent ChangesHermes session lifecycle and project scoping
Sequence Diagram(s)sequenceDiagram
participant Client as initialize()
participant Resolver as _resolve_project()
participant Session as Remote Session
participant MemoryTools as Memory Tools
participant Atexit as atexit Handler
Client->>Resolver: resolve_project(cwd)
Resolver-->>Client: project_id
Client->>Session: POST /session/start(sessionId, project, cwd)
Session-->>Client: session created
Client->>Atexit: register _close_session()
MemoryTools->>Session: POST /memory/recall(project, query, limit)
Session-->>MemoryTools: results
Note over Atexit,Session: On interpreter exit or explicit shutdown:
Atexit->>Atexit: _close_session() idempotent guard
Atexit->>Session: POST /session/end once
Session-->>Atexit: session ended
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
integrations/hermes/__init__.py (3)
26-45: ⚡ Quick winConsider logging git command failures for troubleshooting.
The function silently returns
""on any git failure, which provides graceful degradation but makes it harder to diagnose issues when git is misconfigured or unavailable. Consider adding optional debug logging for the exception case to help users troubleshoot setup problems.💡 Example: Add debug logging
+import logging + +_logger = logging.getLogger(__name__) + def _project_from_remote(path: str) -> str: """Repo name from ``git remote get-url origin``, or "" if unavailable. Parses both remote forms: ``git@host:owner/repo(.git)`` and ``https://host/owner/repo(.git)`` -> ``repo``. """ try: url = subprocess.run( ["git", "-C", path, "remote", "get-url", "origin"], capture_output=True, text=True, timeout=2, ).stdout.strip() - except Exception: + except Exception as e: + _logger.debug("Failed to read git remote from %s: %s", path, e) return ""🤖 Prompt for 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. In `@integrations/hermes/__init__.py` around lines 26 - 45, The _project_from_remote function swallows all exceptions and returns an empty string, making failures hard to diagnose; update the except block in _project_from_remote to optionally log the caught exception (e.g., using the module logger via logging.getLogger(__name__)) at debug or warning level with context like "git remote get-url origin failed for path=%s" before returning "", so callers still get graceful degradation but developers can enable logs to troubleshoot git command failures.
446-463: ⚖️ Poor tradeoffPotential race condition in concurrent teardown (edge case).
Between checking
_session_closed(line 453) and setting it (line 458), concurrent calls could both pass the check and attempt cleanup twice. This is unlikely in the Hermes lifecycle (atexit, on_session_end, and shutdown are typically sequential), but could theoretically happen if shutdown is called from multiple threads.If you want to eliminate the race, use a threading lock. However, given the current usage patterns, this is likely acceptable as-is.
🔒 Optional: Add thread-safe guard
+import threading + class AgentMemoryProvider(MemoryProvider): + def __init__(self): + self._close_lock = threading.Lock() def _close_session(self, session_id: str | None = None) -> None: """Idempotently end the agentmemory session. Every teardown path converges here - on_session_end()/shutdown() on the cli/gateway paths and the atexit hook on the ACP/headless path - so the session is ended exactly once and never left dangling. """ - if getattr(self, "_session_closed", False): - return + with self._close_lock: + if getattr(self, "_session_closed", False): + return + self._session_closed = True sid = session_id or getattr(self, "_session_id", None) if not sid or not getattr(self, "_base", None): return - self._session_closed = True🤖 Prompt for 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. In `@integrations/hermes/__init__.py` around lines 446 - 463, The _close_session method has a small race between checking and setting _session_closed; make it thread-safe by introducing a lock (e.g., an instance threading.Lock or threading.RLock stored as self._session_lock in the initializer) and wrap the check-and-set and early-return logic in a with self._session_lock: block inside _close_session so only one thread can pass the guard and set self._session_closed = True before proceeding to atexit.unregister and calling _api(self._base, "session/end", {"sessionId": sid}).
72-72: 💤 Low valueRemove unnecessary
sorted()call.The order of candidates doesn't affect the logic, so sorting adds unnecessary overhead. When scanning directories with many entries, this could be noticeable.
♻️ Suggested change
- for name in sorted(os.listdir(cwd)): + for name in os.listdir(cwd):🤖 Prompt for 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. In `@integrations/hermes/__init__.py` at line 72, The loop currently iterates using sorted(os.listdir(cwd)), causing unnecessary sorting overhead; change the iteration to use os.listdir(cwd) directly (or use os.scandir(cwd) for better performance on large directories) by removing the sorted() wrapper where the loop over directory entries is formed (the for name in ... loop that references cwd).
🤖 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 `@integrations/hermes/__init__.py`:
- Line 263: The assignment to self._cwd uses "or" which treats an explicit empty
string as false and falls back to os.getcwd(); change the logic in the __init__
so you only fall back when cwd is absent/None (not when it's the empty string)
by checking kwargs["cwd"] or kwargs.get("cwd") for None explicitly and assigning
that value to self._cwd otherwise using os.getcwd(); update the expression
around self._cwd, referencing kwargs["cwd"], kwargs.get("cwd"), and os.getcwd()
accordingly so an explicit "" is preserved.
- Around line 269-273: The session/start API call made via _api(self._base,
"session/start", {...}) is not checked; wrap the call in error handling and
verify its success (catch exceptions from _api or inspect its returned result),
then on failure either raise a clear exception or log an error and abort/return
early so the plugin does not continue with an unstarted session; reference the
session_id, self._project and self._cwd parameters in the error message to aid
debugging.
---
Nitpick comments:
In `@integrations/hermes/__init__.py`:
- Around line 26-45: The _project_from_remote function swallows all exceptions
and returns an empty string, making failures hard to diagnose; update the except
block in _project_from_remote to optionally log the caught exception (e.g.,
using the module logger via logging.getLogger(__name__)) at debug or warning
level with context like "git remote get-url origin failed for path=%s" before
returning "", so callers still get graceful degradation but developers can
enable logs to troubleshoot git command failures.
- Around line 446-463: The _close_session method has a small race between
checking and setting _session_closed; make it thread-safe by introducing a lock
(e.g., an instance threading.Lock or threading.RLock stored as
self._session_lock in the initializer) and wrap the check-and-set and
early-return logic in a with self._session_lock: block inside _close_session so
only one thread can pass the guard and set self._session_closed = True before
proceeding to atexit.unregister and calling _api(self._base, "session/end",
{"sessionId": sid}).
- Line 72: The loop currently iterates using sorted(os.listdir(cwd)), causing
unnecessary sorting overhead; change the iteration to use os.listdir(cwd)
directly (or use os.scandir(cwd) for better performance on large directories) by
removing the sorted() wrapper where the loop over directory entries is formed
(the for name in ... loop that references cwd).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1604e08b-1ca6-40bb-9ed0-a1f7b48d625f
📒 Files selected for processing (1)
integrations/hermes/__init__.py
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.
What
Two independent fixes to the Hermes integration (
integrations/hermes/__init__.py), found by running the plugin in production under a headless ACP-driven Hermes deployment that checks every task out into an ephemeral git worktree. Both are generic - any worktree/CI/orchestrator layout hits them.1. Scope memory by git remote name, not the checkout path (commit 1)
Problem:
initialize()used the session cwd as the agentmemoryprojectid. A checkout path is not a stable repo identity: ephemeral checkouts (git worktrees, CI runners, agent-orchestrator workdirs) place the same repo under throwaway paths, and different repos often share one checkout basename (workdir,workspace, ...). In our deployment every task ran in.../<task-hash>/workdir- all projects collapsed into one sharedworkdirmemory bucket, and recall started returning Go lessons inside TypeScript tasks.Fix: resolve the project id as
AGENTMEMORY_PROJECT_NAMEenv var (explicit override),originremote repo name - of the cwd or of a direct child checkout (covers sessions started one level above the repo),The real cwd is still sent as
cwd, and the resolved project is now passed explicitly on thesearch/remember/smart-searchcalls so recall and save stay inside the project bucket.2. End the session on interpreter exit (commit 2)
Problem: Hermes' cli and gateway paths call
on_session_end()/shutdown(), but the ACP adapter path (headless editors / daemons) tears the process down without either - every such run leaked its agentmemory session as permanently "active" on the server.Fix: register an
atexithook atinitialize()and route all teardown paths (on_session_end,shutdown, atexit) through an idempotent_close_session(), sosession/endis POSTed exactly once on any graceful exit regardless of which host path ran. Hard kills (SIGKILL) can't be caught - that remains the server-side idle sweep's job.How to verify
git worktree add /tmp/wt-a <repo-a>and start a Hermes session there, repeat with a second repo in/tmp/wt-b; with this patchsession/startcarriesproject: <repo-a-name>/project: <repo-b-name>instead ofwt-a/wt-b(or one shared basename). A dir without any git remote falls back to its basename exactly as before.npm run build/npm testunaffected.python3 -m py_compile integrations/hermes/__init__.pypasses;_resolve_project()exercised against the four layouts above (worktree-with-remote, parent-of-checkout, plain dir, env override).Both commits are DCO signed-off. No issue filed for these - happy to split this into two issues/PRs if you prefer one logical change per PR taken strictly.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features