fix(cli): add --data-dir flag + AGENTMEMORY_DATA_DIR so engine state lives outside repos#314
Conversation
|
@mvanhorn is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds configurable data-directory resolution from CLI, environment, and platform defaults. The CLI rewrites engine storage paths, Docker Compose mounts the resolved directory, and tests and documentation cover the behavior. ChangesConfigurable data directory
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DataDirResolver
participant RuntimeConfig
participant IiiEngine
CLI->>DataDirResolver: Resolve CLI, environment, and default path
DataDirResolver-->>CLI: Return dataDir and relocation metadata
CLI->>RuntimeConfig: Rewrite storage paths and write iii-config.yaml
RuntimeConfig-->>CLI: Return runtimeConfigPath
CLI->>IiiEngine: Start with runtimeConfigPath and dataDir
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cli-data-dir.test.ts (1)
1-73: ⚡ Quick winAdd test coverage for git relocation behavior.
The test suite doesn't verify the
relocatedFromfield that's returned when the default data directory is inside a git worktree. Adding tests for this behavior would have caught the logic bug insrc/cli-data-dir.tslines 99-110.📝 Example test to add
it("relocates default when cwd is inside a git repo", () => { const cwd = "/repo/project"; const home = "/home/alex"; const resolved = resolveDataDir({ args: [], env: {}, cwd, home, platform: "linux", }); // Should relocate away from XDG default when inside a git repo expect(resolved.source).toBe("default"); expect(resolved.relocatedFrom).toBeDefined(); });🤖 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/cli-data-dir.test.ts` around lines 1 - 73, The tests are missing coverage for the git relocation behavior: add a test calling resolveDataDir with no args/env and cwd inside a git worktree to assert source === "default" and that relocatedFrom is defined; if that test fails, fix the relocation logic inside resolveDataDir so that when the computed default dataDir would live inside the current cwd (or git worktree) the function moves it out and sets the relocatedFrom property to the original path (ensure the branch that computes the alternate dataDir assigns relocatedFrom before returning). Target the resolveDataDir function and the relocatedFrom field to locate and update the code and add the corresponding test case to the test suite.
🤖 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 `@src/cli-data-dir.ts`:
- Around line 99-110: The relocation check is using nearestGitParent(defaultDir)
but should detect if the current working directory is inside a git repo; change
the condition to call nearestGitParent(cwd) instead of
nearestGitParent(defaultDir) so that the block which computes relocated via
platformDefaultDataDir(env, home, nodePlatform, false) and returns { dataDir:
relocated, source: "default", relocatedFrom: defaultDir } executes when the
process is started inside a git repository; update any variable references
around that if needed to keep the existing logic (defaultDir, relocated) intact.
---
Nitpick comments:
In `@test/cli-data-dir.test.ts`:
- Around line 1-73: The tests are missing coverage for the git relocation
behavior: add a test calling resolveDataDir with no args/env and cwd inside a
git worktree to assert source === "default" and that relocatedFrom is defined;
if that test fails, fix the relocation logic inside resolveDataDir so that when
the computed default dataDir would live inside the current cwd (or git worktree)
the function moves it out and sets the relocatedFrom property to the original
path (ensure the branch that computes the alternate dataDir assigns
relocatedFrom before returning). Target the resolveDataDir function and the
relocatedFrom field to locate and update the code and add the corresponding test
case to the test suite.
🪄 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: e47788fc-09f1-41a2-a834-a3ab49a9e16e
📒 Files selected for processing (5)
README.mddocker-compose.ymlsrc/cli-data-dir.tssrc/cli.tstest/cli-data-dir.test.ts
|
Please be aware of #871 which is about moving the |
|
hey, please rebase. thanks |
|
Would you consider moving the default install directory to $XDG_DATA_HOME/agentmemory also? Otherwise there would be two directories ( |
|
Thanks @HaleTom - good news, the PR already does this. On Linux the default resolves to |
|
The core fix is right. Confirmed on current main: the engine spawns with the caller's cwd and the bundled Two things before it can land:
Supply chain and scope look clean (no dependency, postinstall, or workflow changes). Solid DX win once the migration path is in. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli-data-dir.test.ts (1)
84-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the legacy-adoption assertion to also verify
source.This is the only test exercising the legacy-adoption return branch, but it only checks
.dataDir, not the full result (unlike the second assertion in this test, which uses.toEqualincludingsource). Verifyingsource: "default"here closes a small coverage gap on a newly-added branch.✅ Proposed strengthening
- expect(resolveDataDir({ args: [], env: {}, ...base }).dataDir).toBe( - join(cwd, "data"), - ); + expect(resolveDataDir({ args: [], env: {}, ...base })).toEqual({ + dataDir: join(cwd, "data"), + source: "default", + });🤖 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/cli-data-dir.test.ts` around lines 84 - 86, Strengthen the legacy-adoption assertion for resolveDataDir by verifying the complete result, including source: "default", rather than checking only dataDir. Keep the expected dataDir value unchanged and align this assertion with the existing full-result assertion in the same 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 `@test/cli-data-dir.test.ts`:
- Around line 84-86: Strengthen the legacy-adoption assertion for resolveDataDir
by verifying the complete result, including source: "default", rather than
checking only dataDir. Keep the expected dataDir value unchanged and align this
assertion with the existing full-result assertion in the same test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1814486d-d5a5-4c87-b69b-f4a7dcc0ab3d
📒 Files selected for processing (3)
src/cli-data-dir.tssrc/cli.tstest/cli-data-dir.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli.ts
|
Pushed 8242fab with the adopt-legacy step: if a ./data store from the prior default exists, it keeps being used before the platform-default kicks in, so nobody's memories vanish on upgrade. Added tests covering both the changed-file and no-op paths. Build and vitest pass locally (5/5). |
|
@mvanhorn conflicts are still there. |
…lives outside repos (rohitg00#303) Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Before falling back to the new platform default, detect an existing ./data (prior default) store and keep using it so existing users do not boot into an empty store. Covers both paths with tests.
8242fab to
ef668eb
Compare
|
Rebased onto latest main and resolved the conflicts. The data-dir handling is re-ported onto the refactored engine-start flow: the runtime iii config (pointing at the relocated data dir) now feeds startIiiBin and the pinned-version path, and my old fallback loop is dropped in favor of main's pickCompatibleIii. Build and tests pass locally. Should be clean to review now. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
1229-1229: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken local-models link fragment.
The
#local-models-ollama-lm-studio-vllmfragment does not match the generated heading anchor, so the link is broken. Use the valid generated fragment or add an explicit anchor.🤖 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 `@README.md` at line 1229, Update the Local model row’s link target to match the generated heading anchor for the “Local models” section, or add an explicit anchor that makes the existing fragment valid; keep the displayed link text and surrounding configuration guidance unchanged.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.
Outside diff comments:
In `@README.md`:
- Line 1229: Update the Local model row’s link target to match the generated
heading anchor for the “Local models” section, or add an explicit anchor that
makes the existing fragment valid; keep the displayed link text and surrounding
configuration guidance unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d971912-9271-49ca-b765-d8a567321115
📒 Files selected for processing (5)
README.mddocker-compose.ymlsrc/cli-data-dir.tssrc/cli.tstest/cli-data-dir.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docker-compose.yml
- test/cli-data-dir.test.ts
- src/cli-data-dir.ts
- src/cli.ts
The autogen env block in the agentmemory-config skill reference was stale after adding the --data-dir flag; regenerated via npm run skills:gen so AGENTMEMORY_DATA_DIR is listed (34 -> 35 recognized variables). Fixes the failing skills-reference drift check.
|
Regenerated the skills reference ( |
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Fixed the broken anchor — the heading slug is |
rohitg00
left a comment
rohitg00
left a comment
There was a problem hiding this comment.
Thanks for turning this around, @mvanhorn. The adopt-legacy commit is exactly the migration path I was worried about last time, and the rebase cleared the conflicts. Core idea is right and I want it in. Three things holding me back, all about what happens on real setups:
1. The legacy ./data pickup is too eager. resolveDataDir adopts any data/ folder in the cwd (existsSync(resolve(cwd, "data"))), but loads of repos already have a data/ dir that has nothing to do with us (ML/data projects especially). Someone runs npx @agentmemory/agentmemory in one of those and we quietly start writing state_store.db / stream_store / iii-config.yaml into their folder. That's worse than the bug we're fixing. Can we only adopt it when it's actually ours, e.g. data/state_store.db (or data/iii-config.yaml) exists?
2. Docker users lose their volume. cli.ts sets process.env.AGENTMEMORY_DATA_DIR to the resolved absolute path unconditionally, even for the default. So in docker-compose, ${AGENTMEMORY_DATA_DIR:-iii-data} never falls back to the iii-data named volume, it bind-mounts the platform dir instead. Anyone already on the docker path suddenly boots against an empty dir with their old memories stranded in the named volume. I think we only want to export AGENTMEMORY_DATA_DIR when the user actually passed --data-dir / set the env, so the default keeps the volume.
3. The XDG relocation is too broad. It fires whenever cwd is inside any git repo and XDG_DATA_HOME is set, without checking XDG actually points inside the worktree. So XDG_DATA_HOME=/mnt/data run from a normal repo gets XDG ignored plus a warning that says it's "inside a git worktree" when it isn't. Can we scope it to when the XDG path is really under the git root?
Two smaller ones, fine as follow-ups: --data-dir doesn't move the image store (IMAGES_DIR is still hardcoded to ~/.agentmemory/images, nothing on the worker reads AGENTMEMORY_DATA_DIR), and renderIiiConfig rewrites the two file_path lines by exact string match, so a custom ~/.agentmemory/iii-config.yaml would silently keep writing to ./data.
Requesting changes on 1-3, but this is close and I appreciate you sticking with it.
Addresses the three blocking review items.
1. resolveDataDir only adopts a cwd-local data/ directory when it is actually
ours, keyed on data/state_store.db or data/iii-config.yaml existing. Before,
any data/ folder was adopted, so running the CLI in an unrelated repo that
happens to have one (common in ML projects) would start writing our stores
into it.
2. cli.ts only exports AGENTMEMORY_DATA_DIR when the user actually supplied a
--data-dir flag or env value. Exporting it for the default too meant
${AGENTMEMORY_DATA_DIR:-iii-data} in docker-compose never fell back to the
named volume, so existing docker users booted against an empty bind-mounted
platform dir with their memories stranded in the volume.
3. The XDG relocation now requires the XDG path to actually live under the git
root, rather than firing whenever cwd is inside any repo with XDG_DATA_HOME
set. Previously XDG_DATA_HOME=/mnt/data run from a normal repo was ignored
with a warning claiming it was inside a git worktree when it was not.
The two smaller items you flagged as fine-as-follow-ups (IMAGES_DIR not moving
with --data-dir, and renderIiiConfig rewriting file_path by exact string match)
are untouched here.
|
All three addressed in 9f5f449.
I left the two you called follow-ups alone: Test run: 1417 passed, 5 failed. The 5 are in |
|
Thanks @mvanhorn for your contribution! |
|
Docs PR opened: https://github.com/mintlify-community/docs-agentmemory-abc2951c/pull/3 Documented the new --data-dir CLI flag and AGENTMEMORY_DATA_DIR environment variable, including platform defaults and multi-instance behavior. |
3 participants
Summary
Adds a
--data-dirflag andAGENTMEMORY_DATA_DIRenv var so the engine writes its state outside the caller's cwd. Defaults to a platform-appropriate user dir, so the documentednpx @agentmemory/agentmemoryflow no longer drops adata/tree into whatever repo you happen to be in.What changed
src/cli-data-dir.ts— new resolver: flag > env > platform default.~expansion,${VAR}interpolation,.and./datahonored as explicit opt-ins for repo-local state.src/cli.ts—--data-dir <path>parsed alongside--port; writes a runtimeiii-config.yamlinto the resolved dir sostate_store.dbandstream_storelive there too.--helpdocuments the flag.docker-compose.yml— fallback mount becomes${AGENTMEMORY_DATA_DIR:-iii-data}to/data.README.md— short section on the new flag and env var.test/cli-data-dir.test.ts— covers flag-wins, env-fallback, default-not-cwd, repo-local opt-in,~expansion.Default by platform:
~/Library/Application Support/agentmemory$XDG_DATA_HOME/agentmemory(falls back to~/.local/share/agentmemory)~/AppData/Local/agentmemoryTesting
npm testandnpm run buildcouldn't run locally becausevitest/tsdownaren't installed in this sandbox (npm installblocked by network). The new tests are written against the existing vitest shape used elsewhere intest/; CI / CodeRabbit will exercise them.Fixes #303
Signed-off-by: Matt Van Horn mvanhorn@gmail.com
Summary by CodeRabbit
--data-dirorAGENTMEMORY_DATA_DIR, with OS-specific defaults and--instance-scoped storage.iii-config.yamlso databases and stream files live under the resolved directory (with a one-time relocation notice when applicable)./datapath using theAGENTMEMORY_DATA_DIR-based location instead of a fixed named volume.AGENTMEMORY_DATA_DIR.import-jsonlnow treats--data-diras a value-taking option.