feat(hermes): support explicit Nowledge Mem agent identity#324
Merged
wey-gu merged 19 commits intoJun 24, 2026
Merged
Conversation
…file is default When Hermes uses the default profile, Hermes core passes agent_identity="default" to the memory provider. This makes it impossible to distinguish clients on the Nowledge Mem server when the same server is used by multiple machines. This change adds a fallback: when agent_identity is "default", check nowledge-mem.json for an explicit agent_identity field. This lets each machine set its own agent identity in the plugin config file without changing the Hermes profile. Backward compatible: if nowledge-mem.json has no agent_identity field, behavior is unchanged (remains "default").
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@nowledge-mem-hermes/provider.py`:
- Around line 810-811: The fallback logic that assigns identity from
config.get("agent_identity", identity) at lines 810-811 can assign a non-string
value, which will cause a TypeError when template.replace("{identity}",
identity) is called later. After the identity assignment from the config
fallback, validate that the assigned value is a string. Either convert
non-string values to string using str() conversion or skip the assignment if the
value is not a string, ensuring that only string identities are used for
template substitution.
- Around line 286-289: The config.get("agent_identity", identity) call on line
288 can return non-string JSON values, which then get assigned to host_agent_id
without type normalization, causing inconsistent identity handling. Apply the
same string normalization to the configured agent_identity value that is already
being applied to raw_identity at the beginning of the block: wrap the
config.get("agent_identity", identity) result with str() and .strip() to ensure
the value is always converted to a string before being assigned to
host_agent_id.
🪄 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: 3988f4f0-7389-4a3d-bc86-7df20d604b77
📒 Files selected for processing (1)
nowledge-mem-hermes/provider.py
Replaces the static config-fallback approach with automatic fingerprint generation: 1. /etc/machine-id — standard on systemd/Linux hosts 2. /proc/1/mountinfo — overlay upperdir layer ID for Docker/LazyCat containers On first run the fingerprint is persisted to nowledge-mem.json so it survives restarts without being regenerated. Resolution order: - Hermes named profile identity → use as-is - nowledge-mem.json agent_identity → use explicit override - Auto-generate fingerprint → save to nowledge-mem.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nowledge-mem-hermes/provider.py (1)
886-889: 💤 Low valueNarrow the exception clause to specific expected exceptions.
The bare
except Exception:could mask unexpected errors. Since you're reading JSON from a file, catching(OSError, json.JSONDecodeError)would be more precise while still handling all expected failure modes.Suggested narrower exception handling
try: cfg = json.loads(config_path.read_text(encoding="utf-8")) - except Exception: + except (OSError, json.JSONDecodeError): cfg = {}🤖 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 `@nowledge-mem-hermes/provider.py` around lines 886 - 889, Replace the bare except Exception clause with a more specific exception handler that catches only the expected exceptions. Since the code calls config_path.read_text() and json.loads(), modify the except statement to catch (OSError, json.JSONDecodeError) instead of Exception. This will allow unexpected errors to propagate while still handling the two specific failure modes: file reading errors from read_text and JSON parsing errors from json.loads.Source: Linters/SAST tools
🤖 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 `@nowledge-mem-hermes/provider.py`:
- Around line 931-937: The _resolve_space function signature has changed to
accept identity as a string parameter instead of a dictionary, but the tests in
test_space_resolution.py are still passing dictionaries as the second argument.
Update all test calls to _resolve_space throughout test_space_resolution.py to
extract the identity string value from the dictionary argument (such as using
dictionary.get("agent_identity")) and pass that string directly to
_resolve_space instead of passing the entire dictionary. This will align the
test calls with the new function signature that expects a string parameter which
it immediately calls .strip() on.
---
Nitpick comments:
In `@nowledge-mem-hermes/provider.py`:
- Around line 886-889: Replace the bare except Exception clause with a more
specific exception handler that catches only the expected exceptions. Since the
code calls config_path.read_text() and json.loads(), modify the except statement
to catch (OSError, json.JSONDecodeError) instead of Exception. This will allow
unexpected errors to propagate while still handling the two specific failure
modes: file reading errors from read_text and JSON parsing errors from
json.loads.
🪄 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: 492679f5-b82d-49d7-b96e-c86f6fa8772b
📒 Files selected for processing (1)
nowledge-mem-hermes/provider.py
_resolve_space now accepts (config, identity: str) instead of (config, kwargs: dict). Update all test call sites to pass the identity string directly.
KingBoyAndGirl
changed the title
… not yet supported)
KingBoyAndGirl
changed the title
…oks (CLI not yet supported)" This reverts commit f39e50b.
…oks (pending nmem CLI support for --host-agent-id on t save)
KingBoyAndGirl
changed the title
…g nmem CLI support for --host-agent-id on t import)
Replace the overlay-only fallback with a universal chain that works across bare metal, VMs, Docker, and LPK: 1. /etc/machine-id - systemd hosts (gold standard) 2. MAC address - all Linux environments (Docker assigns per-host IP-to-MAC, unique across machines in practice) 3. overlay - /proc/1/mountinfo layer hash (last resort) Adds _read_primary_mac() that scans /sys/class/net/*/address, skipping loopback. Different prefixes (hermes-/overlay-) identify which source was used.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
nowledge-mem-codex-plugin/hooks/nmem-stop-save.py (1)
205-222:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Overlay ID extraction returns non-unique value.
Same bug as in
nmem-hook-save.py:Path(upperdir).namereturns"diff"for all containers instead of the unique layer ID.🐛 Fix to extract the unique layer ID
- return Path(upperdir).name + return Path(upperdir).parent.name🤖 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 `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py` around lines 205 - 222, The _extract_overlay_id function incorrectly extracts the overlay ID by using Path(upperdir).name which returns only the final directory component ("diff") for all containers instead of the unique layer ID. Replace the line that uses Path(upperdir).name with a path traversal that extracts the unique layer identifier from a parent directory in the path hierarchy, likely using .parent.name or navigating up multiple levels to get the actual layer ID rather than just the final directory name.nowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.py (1)
228-244:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Overlay ID extraction returns non-unique value.
Same bug as in the other hook files:
Path(value[:end]).namereturns"diff"for all containers instead of the unique layer ID.🐛 Fix to extract the unique layer ID
- return Path(value[:end]).name + return Path(value[:end]).parent.name🤖 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 `@nowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.py` around lines 228 - 244, The _extract_overlay_id function has a critical bug in its return statement where Path(value[:end]).name extracts only the filename portion (which is "diff") instead of the unique layer ID. Instead of using .name to get the final path component, extract the parent directory of the "diff" path to get the actual unique layer identifier. You can achieve this by using Path operations to access the parent directory and then getting its name, which will give you the unique layer hash instead of the generic "diff" string.
🤖 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 `@nowledge-mem-claude-code-plugin/scripts/nmem-hook-save.py`:
- Around line 137-154: The function `_extract_overlay_id` returns the final path
component (which is always "diff" for Docker overlay2 containers) instead of the
unique layer ID, causing all containers to have identical fingerprints. In the
return statement, instead of using `Path(upperdir).name` which returns the final
component, access the parent directory and then get its name to extract the
unique layer ID from the Docker overlay2 structure. This will return the actual
unique identifier between the `/overlay2/` and `/diff` directories.
In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 187-224: The functions _host_agent_fingerprint and
_extract_overlay_id are duplicated with nearly identical implementations across
three hook files (nmem-hook-save.py, nmem-stop-save.py, and
copilot-stop-save.py). Create a new shared utility module for fingerprinting
logic and move both _host_agent_fingerprint and _extract_overlay_id functions
into this shared module. Then update all three hook files to import and use
these functions from the shared utility module instead of maintaining duplicate
copies.
---
Duplicate comments:
In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 205-222: The _extract_overlay_id function incorrectly extracts the
overlay ID by using Path(upperdir).name which returns only the final directory
component ("diff") for all containers instead of the unique layer ID. Replace
the line that uses Path(upperdir).name with a path traversal that extracts the
unique layer identifier from a parent directory in the path hierarchy, likely
using .parent.name or navigating up multiple levels to get the actual layer ID
rather than just the final directory name.
In `@nowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.py`:
- Around line 228-244: The _extract_overlay_id function has a critical bug in
its return statement where Path(value[:end]).name extracts only the filename
portion (which is "diff") instead of the unique layer ID. Instead of using .name
to get the final path component, extract the parent directory of the "diff" path
to get the actual unique layer identifier. You can achieve this by using Path
operations to access the parent directory and then getting its name, which will
give you the unique layer hash instead of the generic "diff" string.
🪄 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: 6b7949a1-df2d-423c-8015-1b5fc1ece88d
📒 Files selected for processing (5)
nowledge-mem-claude-code-plugin/scripts/nmem-hook-save.pynowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.pynowledge-mem-hermes/provider.pynowledge-mem-hermes/tests/test_space_resolution.py
🚧 Files skipped from review as they are similar to previous changes (1)
- nowledge-mem-hermes/provider.py
- Fix _extract_overlay_id: Path().name -> Path().parent.name was returning diff for all containers instead of unique layer ID - Replace bare except Exception with (OSError, json.JSONDecodeError) in _save_agent_identity
Contributor
Author
变更摘要本 PR 为 nowledge-mem 的四个客户端插件（Hermes、Codex、Claude Code/Grok、Copilot CLI）增加了自动 agent 身份指纹识别功能。 核心逻辑三级 fallback 指纹生成：
指纹前缀规则：
行为
端到端验证（脱敏）在两台 Docker 容器部署的 Hermes Agent 上验证：
验证要点：
已知限制
|
Contributor
Author
|
补充说明一下这里的实现原因： 这里的问题不在于“开源/闭源”本身，而在于当前 当前验证结果（
因此，这个 PR 里多个插件分别实现指纹逻辑，并不是因为更喜欢分散实现，而是因为目前还没有一个 CLI 侧的统一入口可复用。等 CLI 后续补上 thread save/import 的 host-agent-id 支持后，这部分逻辑可以进一步统一和收敛。 Clarification on the implementation rationale: The issue here is not really “open source vs closed source” by itself. The practical constraint is that the current Current verified status on
So the fingerprint logic is currently duplicated across several plugins not because this is the preferred design, but because there is not yet a reusable CLI-level identity propagation entry point for these flows. Once the CLI adds host-agent-id support for thread save/import operations, this logic can be consolidated further. |
Co-authored-by: mo <48237066+KingBoyAndGirl@users.noreply.github.com>
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesAgent Identity Resolution Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
wey-gu
changed the title
Member
|
Thanks @KingBoyAndGirl for digging into the multi-agent identity gap and validating the Hermes case in real container setups. I reshaped the implementation to match Mem's identity model: explicit AI Identity first, advanced external aliases when needed, and no silent machine-fingerprint identity. Your PR is still the one that surfaced and carries this fix. |
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
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.
Summary
Thanks @KingBoyAndGirl for surfacing the missing agent-identity propagation path for multi-agent Hermes/Codex/Claude/Copilot setups. The underlying issue was real: integrations need a reliable way to pass identity context into Nowledge Mem instead of collapsing every long-running agent into the default profile.
This PR now keeps the contribution under the product design we settled on:
agent_id/NMEM_AGENT_IDis the normal portable Nowledge AI Identity.host_agent_id/NMEM_HOST_AGENT_IDis an advanced external alias for integration authors, not a second user-facing identity.Final behavior
Hermes now resolves identity in this order:
agent_identityfrom runtime kwargs, unless it isdefault.agent_identityinnowledge-mem.json.The resolved identity is used for Context Bundle and
space_by_identity/space_templateresolution. The provider does not auto-generate or persist machine/container fingerprints.Codex, Claude Code, and Copilot hook-side fingerprinting was removed from the final diff. The CLI now provides the right shared path for thread writes:
nmem t save/import/synccan accept explicit identity context and also honorNMEM_AGENT_ID/NMEM_HOST_AGENT_ID.Verification
Co-authored-by: mo 48237066+KingBoyAndGirl@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes