Repository navigation
Conversation
Signed-off-by: Zavian <36817799+Zavianx@users.noreply.github.com>
|
@Zavianx is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds support for loading watcher configuration from ChangesEnvironment file configuration loading
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
integrations/filesystem-watcher/watcher.mjs (3)
61-80: ⚡ Quick winDocument the precedence between config file locations.
When both
~/.agentmemory/.envand$XDG_CONFIG_HOME/agentmemory/.envexist, the implementation gives precedence to the XDG location (line 73'sObject.assignmerges HOME first, then XDG, so XDG values override). This follows the typical XDG pattern (XDG as primary, ~/.appname as legacy), but it's not documented in the README.Users might expect their home directory config to be most specific, or may not be aware that values can be overridden by the XDG location.
Verify the intended precedence order and update the README to clarify which file takes precedence when both exist. Consider adding this to the configuration table:
The watcher loads configuration from these locations in order (later sources override earlier): 1. `~/.agentmemory/.env` 2. `$XDG_CONFIG_HOME/agentmemory/.env` 3. Process environment variables🤖 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/filesystem-watcher/watcher.mjs` around lines 61 - 80, The loadAgentMemoryEnv function builds candidates with HOME first then XDG and merges with Object.assign so XDG values currently override HOME; verify this is the intended precedence and update the README configuration section to clearly document the load/override order (e.g., list ~/.agentmemory/.env then $XDG_CONFIG_HOME/agentmemory/.env, with later sources overriding earlier, and that process env overrides both), referencing loadAgentMemoryEnv and the candidates/Object.assign behavior so users know XDG takes precedence when both files exist.
38-59: ⚡ Quick winConsider adding test coverage for env file edge cases.
The
parseEnvFileimplementation handles quotes, inline comments (#pattern), and malformed lines gracefully, but the test file only exercises basicKEY=valuepairs. Consider adding tests for:
- Quoted values:
KEY="value with spaces"- Inline comments:
KEY=value # comment- Missing closing quotes:
KEY="incomplete- Empty values:
KEY=🤖 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/filesystem-watcher/watcher.mjs` around lines 38 - 59, Add unit tests covering edge cases for parseEnvFile: create tests that call parseEnvFile with content including quoted values (e.g., KEY="value with spaces") and assert the value preserves spaces without quotes; inline comments (e.g., KEY=value # comment) and assert the comment is stripped; a missing closing quote (e.g., KEY="incomplete) and assert the parser returns the remainder after the opening quote as the value; and empty values (e.g., KEY=) and assert the value is an empty string. Target the parseEnvFile function in your test suite and include assertions for each of these scenarios to match the existing parsing behavior.
61-80: ⚡ Quick winAdd test coverage for XDG_CONFIG_HOME loading.
The test suite validates loading from
~/.agentmemory/.env(lines 195-220 in test file) but doesn't cover$XDG_CONFIG_HOME/agentmemory/.env. Consider adding a test case that:
- Sets both HOME and XDG_CONFIG_HOME
- Places different values in both locations
- Verifies XDG values take precedence per the implementation's Object.assign order
🤖 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/filesystem-watcher/watcher.mjs` around lines 61 - 80, Add a unit test for loadAgentMemoryEnv that sets both HOME and XDG_CONFIG_HOME environment variables, creates two temporary env files (~/.agentmemory/.env and $XDG_CONFIG_HOME/agentmemory/.env) with different key values, invokes loadAgentMemoryEnv (or the module that calls it) and asserts the returned object contains the values from the XDG file overriding the HOME file per Object.assign order; reference the loadAgentMemoryEnv function and the candidates array when locating where to call it, and ensure cleanup of temp files and restored env vars after the test.
🤖 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.
Nitpick comments:
In `@integrations/filesystem-watcher/watcher.mjs`:
- Around line 61-80: The loadAgentMemoryEnv function builds candidates with HOME
first then XDG and merges with Object.assign so XDG values currently override
HOME; verify this is the intended precedence and update the README configuration
section to clearly document the load/override order (e.g., list
~/.agentmemory/.env then $XDG_CONFIG_HOME/agentmemory/.env, with later sources
overriding earlier, and that process env overrides both), referencing
loadAgentMemoryEnv and the candidates/Object.assign behavior so users know XDG
takes precedence when both files exist.
- Around line 38-59: Add unit tests covering edge cases for parseEnvFile: create
tests that call parseEnvFile with content including quoted values (e.g.,
KEY="value with spaces") and assert the value preserves spaces without quotes;
inline comments (e.g., KEY=value # comment) and assert the comment is stripped;
a missing closing quote (e.g., KEY="incomplete) and assert the parser returns
the remainder after the opening quote as the value; and empty values (e.g.,
KEY=) and assert the value is an empty string. Target the parseEnvFile function
in your test suite and include assertions for each of these scenarios to match
the existing parsing behavior.
- Around line 61-80: Add a unit test for loadAgentMemoryEnv that sets both HOME
and XDG_CONFIG_HOME environment variables, creates two temporary env files
(~/.agentmemory/.env and $XDG_CONFIG_HOME/agentmemory/.env) with different key
values, invokes loadAgentMemoryEnv (or the module that calls it) and asserts the
returned object contains the values from the XDG file overriding the HOME file
per Object.assign order; reference the loadAgentMemoryEnv function and the
candidates array when locating where to call it, and ensure cleanup of temp
files and restored env vars after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb56847d-9954-4ff6-ae15-f3bd47b125d0
📒 Files selected for processing (3)
integrations/filesystem-watcher/README.mdintegrations/filesystem-watcher/watcher.mjstest/fs-watcher.test.ts
Signed-off-by: Zavian <36817799+Zavianx@users.noreply.github.com>
The watcher built its config from process.env alone, so settings in the env file were ignored. bin.mjs now merges the file under process.env, which wins, matching the daemon. Fixes the diagnosis in rohitg00#319. Generated-By: Claude Sonnet 5.5
Summary
~/.agentmemory/.envand$XDG_CONFIG_HOME/agentmemory/.envin@agentmemory/fs-watcherThis addresses the filesystem watcher part of #298, where a watcher running under a process manager/container could fall back to
localhosteven though the shared server URL lives in agentmemory's documented env file.Conflict check
integrations/filesystem-watcher/*ortest/fs-watcher.test.ts.Tests
npm test -- test/fs-watcher.test.tsnode --check integrations/filesystem-watcher/watcher.mjsgit diff --checknpm run buildnpm testSummary by CodeRabbit
Documentation
New Features
Tests