fix(hermes): seed AGENTMEMORY_URL default at import (closes #520)#643
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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)
📝 WalkthroughWalkthroughThe Hermes plugin now ensures ChangesHermes plugin environment initialization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Hermes was reporting agentmemory as Missing/not-available even when the
service was running fine and live sessions could use it. Root cause:
`hermes memory status` reads os.environ in the Hermes CLI process. When
agentmemory is launched by systemd (or any process manager that loads
~/.agentmemory/.env via EnvironmentFile=), those values never reach the
interactive shell. Our plugin already preloads ~/.agentmemory/.env via
setdefault, but only catches uncommented key=value lines. Users whose
.env file has AGENTMEMORY_URL commented out (because it matches the
default http://localhost:3111) end up with the var unset, and Hermes
reports Missing.
Fix: after preload, os.environ.setdefault('AGENTMEMORY_URL',
DEFAULT_BASE_URL). Guarantees the var is set to the documented default
so Hermes status reflects the real runtime. Anything explicit from the
shell or .env still wins (setdefault is no-op when value is present).
AGENTMEMORY_SECRET stays untouched — it's optional per the config
schema.
7803fa9 to
7279aeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/hermes-plugin.test.ts (1)
65-70: ⚡ Quick winStrengthen this regression test to assert import-time execution too.
This currently verifies source text only. Add a second assertion that
_preload_agentmemory_dotenv()is invoked at module scope so the “at import time” contract is covered.Proposed test hardening
it("preloads AGENTMEMORY_URL default at import time (`#520`)", () => { const source = readFileSync("integrations/hermes/__init__.py", "utf8"); expect(source).toMatch( /os\.environ\.setdefault\(\s*["']AGENTMEMORY_URL["']\s*,\s*DEFAULT_BASE_URL\s*\)/, ); + expect(source).toMatch(/^\s*_preload_agentmemory_dotenv\(\)\s*$/m); });🤖 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 `@test/hermes-plugin.test.ts` around lines 65 - 70, The test currently only checks source text for setting AGENTMEMORY_URL; add a second assertion that the helper function _preload_agentmemory_dotenv() is invoked at module scope to prove import-time execution. Locate integrations/hermes/__init__.py and in test/hermes-plugin.test.ts add an assertion that the module contains a top-level call to _preload_agentmemory_dotenv (e.g. a regex match for /_preload_agentmemory_dotenv\(\s*\)/ or import the module and verify side-effect behavior), so both the os.environ.setdefault(...) and the actual invocation of _preload_agentmemory_dotenv are asserted.
🤖 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`:
- Around line 93-97: The current use of os.environ.setdefault("AGENTMEMORY_URL",
DEFAULT_BASE_URL) only sets the default when the key is missing but doesn't
handle cases where AGENTMEMORY_URL exists but is empty or a placeholder like
"${AGENTMEMORY_URL}"; update the logic (around os.environ.setdefault usage in
integrations/hermes/__init__.py) to treat empty strings and common placeholder
patterns as unset by: read os.environ.get("AGENTMEMORY_URL"), and if the value
is falsy or matches placeholder patterns (e.g., "${AGENTMEMORY_URL}" or only
whitespace), assign DEFAULT_BASE_URL to os.environ["AGENTMEMORY_URL"] so the
rest of the code uses the documented default.
---
Nitpick comments:
In `@test/hermes-plugin.test.ts`:
- Around line 65-70: The test currently only checks source text for setting
AGENTMEMORY_URL; add a second assertion that the helper function
_preload_agentmemory_dotenv() is invoked at module scope to prove import-time
execution. Locate integrations/hermes/__init__.py and in
test/hermes-plugin.test.ts add an assertion that the module contains a top-level
call to _preload_agentmemory_dotenv (e.g. a regex match for
/_preload_agentmemory_dotenv\(\s*\)/ or import the module and verify side-effect
behavior), so both the os.environ.setdefault(...) and the actual invocation of
_preload_agentmemory_dotenv are asserted.
🪄 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: eda5b2b5-8970-4213-8f21-2eca90d1e423
📒 Files selected for processing (4)
README.mddocs/recipes/pairings.mdintegrations/hermes/__init__.pytest/hermes-plugin.test.ts
| # Guarantee AGENTMEMORY_URL is set so `hermes memory status` never | ||
| # reports it as Missing when a user runs agentmemory at the default | ||
| # localhost:3111 (or via systemd with the URL line commented out in | ||
| # ~/.agentmemory/.env because it matches the default). #520. | ||
| os.environ.setdefault("AGENTMEMORY_URL", DEFAULT_BASE_URL) |
There was a problem hiding this comment.
Treat empty/placeholder AGENTMEMORY_URL as unset before defaulting.
setdefault only applies when the key is missing. If AGENTMEMORY_URL is present but empty (or a literal ${AGENTMEMORY_URL}), Hermes can still report unavailable instead of using the documented default.
Proposed fix
- os.environ.setdefault("AGENTMEMORY_URL", DEFAULT_BASE_URL)
+ raw_url = (os.environ.get("AGENTMEMORY_URL") or "").strip()
+ if not raw_url or (raw_url.startswith("${") and raw_url.endswith("}")):
+ os.environ["AGENTMEMORY_URL"] = DEFAULT_BASE_URL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Guarantee AGENTMEMORY_URL is set so `hermes memory status` never | |
| # reports it as Missing when a user runs agentmemory at the default | |
| # localhost:3111 (or via systemd with the URL line commented out in | |
| # ~/.agentmemory/.env because it matches the default). #520. | |
| os.environ.setdefault("AGENTMEMORY_URL", DEFAULT_BASE_URL) | |
| # Guarantee AGENTMEMORY_URL is set so `hermes memory status` never | |
| # reports it as Missing when a user runs agentmemory at the default | |
| # localhost:3111 (or via systemd with the URL line commented out in | |
| # ~/.agentmemory/.env because it matches the default). `#520`. | |
| raw_url = (os.environ.get("AGENTMEMORY_URL") or "").strip() | |
| if not raw_url or (raw_url.startswith("${") and raw_url.endswith("}")): | |
| os.environ["AGENTMEMORY_URL"] = DEFAULT_BASE_URL |
🤖 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 93 - 97, The current use of
os.environ.setdefault("AGENTMEMORY_URL", DEFAULT_BASE_URL) only sets the default
when the key is missing but doesn't handle cases where AGENTMEMORY_URL exists
but is empty or a placeholder like "${AGENTMEMORY_URL}"; update the logic
(around os.environ.setdefault usage in integrations/hermes/__init__.py) to treat
empty strings and common placeholder patterns as unset by: read
os.environ.get("AGENTMEMORY_URL"), and if the value is falsy or matches
placeholder patterns (e.g., "${AGENTMEMORY_URL}" or only whitespace), assign
DEFAULT_BASE_URL to os.environ["AGENTMEMORY_URL"] so the rest of the code uses
the documented default.
Summary
Closes #520. Hermes was reporting agentmemory as
Missing/not availableeven when the service was running fine and live Hermes sessions could use it.Root cause
hermes memory statusreadsos.environin the Hermes CLI process. When agentmemory is launched by systemd or any process manager that loads~/.agentmemory/.envviaEnvironmentFile=, those values never reach the interactive shell.Our plugin already preloads
~/.agentmemory/.envviasetdefaultat import time, but only catches uncommentedkey=valuelines. Users whose.envhasAGENTMEMORY_URLcommented out (because it matches the defaulthttp://localhost:3111) end up with the var unset, and Hermes reportsMissing.Fix
One line at the end of
_preload_agentmemory_dotenv():Guarantees
AGENTMEMORY_URLis set to the documented default after import, sohermes memory statusreflects the real runtime instead of the absence of a shell env var. Anything explicit from the shell or.envstill wins —setdefaultis a no-op when the value is already present.AGENTMEMORY_SECRETstays untouched. It's optional perget_config_schema()("required": False); Hermes reporting an optional field asMissingis a separate upstream UI question.Test plan
test/hermes-plugin.test.tsmatches thesetdefaultline in__init__.pynpm test— 1097 / 1097 pass (99 files), no regressionpython3 -c "import hermes; print(os.environ['AGENTMEMORY_URL'])"with emptyHOMEreturnshttp://localhost:3111python3 -c "AGENTMEMORY_URL=http://10.0.0.5:3111 import hermes; print(os.environ['AGENTMEMORY_URL'])"still returns10.0.0.5— explicit env winsNotes for #520 reporter
The fix also makes
is_available()returnTruefor default-localhost setups, since_validate_url("http://localhost:3111")succeeds. The user-suggested Authorization-header patch is a separate path —hermes memory statusdoes its own health check inside Hermes core, not via our plugin. If that health check fails for users withAGENTMEMORY_SECRETset, that's a Hermes-side fix; happy to file an upstream issue if confirmed.Summary by CodeRabbit
Bug Fixes
Tests