feat(cli): materialize ~/.agentmemory/config/iii-config.yaml with absolute paths#699
feat(cli): materialize ~/.agentmemory/config/iii-config.yaml with absolute paths#699m1dm4n wants to merge 1 commit into
Conversation
…olute paths The bundled iii-config.yaml declares `file_path: ./data/state_store.db` and `file_path: ./data/stream_store` as relative paths. When the engine spawns with a different working directory — common under systemd or any launcher that doesn't `cd` into ~/.agentmemory first — the KV state and stream store land wherever cwd points. A user running agentmemory globally under systemd ended up with three independent data dirs (~/Tara/data, ~/data, ~/.agentmemory/data) and lost session history to the unused ones until manual migration. On first start, agentmemory now materializes the bundled template to ~/.agentmemory/config/iii-config.yaml with the two known relative `./data/` paths rewritten to absolute paths under ~/.agentmemory/data/, making the engine location-independent. The helper is wired into both `runInit()` (explicit user-facing init flow) and `startEngine()` (catches users who skip init). The subdir placement also sidesteps a separate failure mode: iii's config watcher (Rust `notify` crate, NonRecursive) watches the PARENT directory of the config file to catch atomic-rename writes. With the config at ~/.agentmemory/iii-config.yaml, every write to sibling files (preferences.json, iii.pid, worker.pid, engine-state.json) triggers a config reload, which drops the worker's HTTP function registrations and 404s every /agentmemory/* route. The quiet config/ subdir has no such siblings. Design notes: - Idempotent: the helper short-circuits if the target file already exists. User edits are preserved. - Shape-guarded: the relative-path regex only fires when both recognizable bundled markers are present. Unknown shapes are copied verbatim — never silently corrupted. - Best-effort: any failure (read-only $HOME, missing bundled, permission denied) is logged via vlog and returns null. The caller falls through to legacy candidates so the engine still starts. - Atomic write: tmp + fsync + rename, mirroring writePrefs() in src/cli/preferences.ts. - No new dependency: uses a regex over the YAML text rather than adding js-yaml for a two-substring rewrite. YAML round-trip would also strip the substantive bundled comments (the `sampling_ratio: 0.1` block documents the 137 GB log feedback loop fix from rohitg00#519). The legacy ~/.agentmemory/iii-config.yaml candidate is kept in the findIiiConfig() resolution order (just below the new subdir target) so existing users with hand-rolled top-level configs keep working unchanged. AGENTMEMORY_III_CONFIG env and cwd-local config still take precedence. Tests: 6 new cases in test/cli-iii-config.test.ts cover materialization, idempotency, unrecognized shape, missing bundled, read-only target, and parallel callers. Signed-off-by: m1dm4n <92845822+m1dm4n@users.noreply.github.com>
|
@m1dm4n is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements first-start generation of a user-anchored ChangesUser-anchored engine config materialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![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 (2)
src/cli/iii-config.ts (1)
1-37: ⚡ Quick winTrim descriptive WHAT-comments in implementation files.
This header is mostly narrative/behavior documentation; keep that in README/ADR and leave only minimal intent comments inline.
As per coding guidelines, "Do not use code comments explaining WHAT — use clear naming instead".
🤖 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 `@src/cli/iii-config.ts` around lines 1 - 37, Remove the long narrative header block at the top of src/cli/iii-config.ts and replace it with a very short intent comment (one or two lines) that states the purpose (e.g., "Materialize user-anchored iii-engine config to an absolute, engine-independent location"). Keep any necessary mentions of the key behaviors only as succinct notes if needed, and do not duplicate WHAT-level documentation — leave detailed rationale/behavior to README/ADR; ensure references to unique symbols like findBundled, prefsDir(), and writePrefs() remain in code or tests but are not explained in-line.src/cli.ts (1)
795-800: ⚡ Quick winPrefer self-documenting naming over WHAT-comments here.
This added block explains mechanics already reflected by
ensureUserIiiConfig()+findIiiConfig()flow; consider removing or shrinking to a short intent note.As per coding guidelines, "Do not use code comments explaining WHAT — use clear naming instead".
🤖 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 `@src/cli.ts` around lines 795 - 800, Remove the long WHAT-style comment block above ensureUserIiiConfig(); if you need any comment at all replace it with a short intent note (one line) describing why we call ensureUserIiiConfig (e.g., "Ensure per-user III config is materialized so findIiiConfig() can locate it") or rely on the clear function name; do not restate mechanics already expressed by ensureUserIiiConfig() and findIiiConfig().
🤖 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 `@test/cli-iii-config.test.ts`:
- Around line 151-152: This test uses require("node:fs") to get chmodSync inside
the test body which breaks in ESM; replace the runtime require with a top-level
ESM import (e.g., import { chmodSync } from "node:fs") and remove the inline
require calls, and update any other occurrences (the second require around the
later chmodSync use) so both uses reference the imported chmodSync; keep the
existing calls to chmodSync(targetDir, 0o500) and chmodSync(targetDir, 0o700)
unchanged.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 795-800: Remove the long WHAT-style comment block above
ensureUserIiiConfig(); if you need any comment at all replace it with a short
intent note (one line) describing why we call ensureUserIiiConfig (e.g., "Ensure
per-user III config is materialized so findIiiConfig() can locate it") or rely
on the clear function name; do not restate mechanics already expressed by
ensureUserIiiConfig() and findIiiConfig().
In `@src/cli/iii-config.ts`:
- Around line 1-37: Remove the long narrative header block at the top of
src/cli/iii-config.ts and replace it with a very short intent comment (one or
two lines) that states the purpose (e.g., "Materialize user-anchored iii-engine
config to an absolute, engine-independent location"). Keep any necessary
mentions of the key behaviors only as succinct notes if needed, and do not
duplicate WHAT-level documentation — leave detailed rationale/behavior to
README/ADR; ensure references to unique symbols like findBundled, prefsDir(),
and writePrefs() remain in code or tests but are not explained in-line.
🪄 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: c9db2a1e-c076-402e-943b-9c4ac2956764
📒 Files selected for processing (4)
README.mdsrc/cli.tssrc/cli/iii-config.tstest/cli-iii-config.test.ts
| const { chmodSync } = require("node:fs"); | ||
| chmodSync(targetDir, 0o500); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ESM mode + CJS require usage in this test file.
set -euo pipefail
echo "== package.json module type =="
fd -a '^package\.json$' | while read -r f; do
echo "-- $f"
jq -r '.type // "(no type field)"' "$f"
done
echo
echo "== require(...) occurrences in test/cli-iii-config.test.ts =="
rg -n 'require\(' test/cli-iii-config.test.ts
echo
echo "== tsconfig module settings (if present) =="
fd -a '^tsconfig.*\.json$' | while read -r f; do
echo "-- $f"
jq -r '.compilerOptions.module // "(no module setting)"' "$f" 2>/dev/null || true
doneRepository: rohitg00/agentmemory
Length of output: 753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File header/imports (top 80 lines) =="
sed -n '1,80p' test/cli-iii-config.test.ts | cat -n
echo
echo "== Context around lines 130-190 =="
sed -n '120,190p' test/cli-iii-config.test.ts | cat -nRepository: rohitg00/agentmemory
Length of output: 5935
Replace require("node:fs") with ESM chmodSync import in test/cli-iii-config.test.ts.
test/cli-iii-config.test.ts is an ESM test (top-level ESM imports) and the repo sets "type": "module", but it uses require("node:fs") inside the test body (lines 151-152 and 165-166). This can fail at runtime in Node ESM (require undefined) and makes the suite sensitive to runner/module mode.
💡 Proposed fix
import {
existsSync,
+ chmodSync,
mkdirSync,
mkdtempSync,
readFileSync,
rmSync,
statSync,
writeFileSync,
} from "node:fs";
@@
- const { chmodSync } = require("node:fs");
chmodSync(targetDir, 0o500);
@@
- const { chmodSync } = require("node:fs");
chmodSync(targetDir, 0o700);📝 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.
| const { chmodSync } = require("node:fs"); | |
| chmodSync(targetDir, 0o500); | |
| chmodSync(targetDir, 0o500); |
🤖 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-iii-config.test.ts` around lines 151 - 152, This test uses
require("node:fs") to get chmodSync inside the test body which breaks in ESM;
replace the runtime require with a top-level ESM import (e.g., import {
chmodSync } from "node:fs") and remove the inline require calls, and update any
other occurrences (the second require around the later chmodSync use) so both
uses reference the imported chmodSync; keep the existing calls to
chmodSync(targetDir, 0o500) and chmodSync(targetDir, 0o700) unchanged.
1 participant
Problem
Two distinct failures, same root cause: agentmemory's iii-engine config layout is anchored to the engine's working directory instead of to
~/.agentmemory/.1. Scattered data directories. Bundled
iii-config.yaml(top of repo) declares:When
iii --config <path>runs with a different cwd — common under systemd or any launcher that doesn'tcdinto~/.agentmemoryfirst — the KV state and stream store land wherever cwd points. A user running agentmemory globally under systemd ended up with three independentdata/dirs (~/Tara/data,~/data,~/.agentmemory/data) and lost session history to the unused ones until manual migration.2. Reload storm → blank viewer. iii's config watcher (Rust
notifycrate,NonRecursive) watches the PARENT directory of the config file to catch atomic-rename writes. With the config at~/.agentmemory/iii-config.yaml, every write to sibling files (preferences.json,iii.pid,worker.pid,engine-state.json) triggers a config reload, which drops the worker's HTTP function registrations →404on every/agentmemory/*route → blank viewer until restart.Fix
On first start, materialize
~/.agentmemory/config/iii-config.yamlfrom the bundled template:./data/paths are rewritten to absolute paths under~/.agentmemory/data/so the engine is location-independent.config/subdir is quiet (no sibling pidfiles, no JSON state files written during normal operation), so iii's parent-dir watcher doesn't fire on every routine write.Helper wired into both call sites:
runInit()(explicitagentmemory initflow) — emits ap.log.successline.startEngine()(catches users who skipinit; theexistsSyncshort-circuit makes it ~free on every subsequent invocation).Backwards compatibility
findIiiConfig()resolution order (post-change):AGENTMEMORY_III_CONFIGenv (highest precedence)<cwd>/iii-config.yaml~/.agentmemory/config/iii-config.yaml← new materialized location~/.agentmemory/iii-config.yaml← legacy, kept__dirname/iii-config.yaml← bundled__dirname/../iii-config.yamlExisting users with hand-rolled
~/.agentmemory/iii-config.yamlkeep working unchanged; the legacy candidate is still consulted, just below the new subdir target.Design notes
existsSync(userPath); user edits are preserved on subsequent starts.file_path: ./data/state_store.dbandfile_path: ./data/stream_store) are present. Unknown shapes are copied verbatim — we never silently corrupt a user-supplied template.$HOME, missing bundled, permission denied) is logged viavlogand returnsnull. The caller falls through to legacy candidates so the engine still starts.tmp + fsync + rename, mirroringwritePrefs()insrc/cli/preferences.ts. Parallel callers on first start produce identical content, so the loser of the rename race leaves no half-written file behind.js-yamlfor a two-substring rewrite. Ajs-yamlround-trip would also strip the substantive bundled comments (thesampling_ratio: 0.1block documents the 137 GB log feedback loop fix from Runaway daemon.log.new growth from observability subscriber-lagged WARN feedback loop #519).materializeUserIiiConfig()lives insrc/cli/iii-config.tsand takestargetDir,dataDir,findBundled,onWarnas args — no implicit dependencies onprefsDir()/__dirname. The thin wrapper insrc/cli.tswires production values; tests inject their own.Tests
6 new cases in
test/cli-iii-config.test.ts:findBundlednow returns something differentonWarninvokednull, no file writtennull,onWarninvoked, no crashPromise.all)Full suite:
Test Files 4 failed | 114 passed (118)/Tests 8 failed | 1266 passed (1274). The 8 failures (test/auto-compress.test.ts,test/embedding-provider.test.ts,test/mcp-standalone*.test.ts) are present onmainat HEAD before this branch is applied — they are pre-existing and unrelated.How to verify
Re-run
node dist/cli.mjs initand confirm the materialized file's mtime is unchanged.Out of scope (possible follow-ups)
paths.tsconsolidation.~/.agentmemory/...is joined ad-hoc in ~10 sites (src/cli.ts,src/config.ts,src/cli/preferences.ts,src/index.ts, etc.). Centralizing them into one accessor would touch too many files for this PR; happy to do it as a follow-up if a maintainer wants.NonRecursivewatch is by design (to catch atomic-rename writes); a fix would need to change either the watch scope or the reload-on-event filter. That belongs iniii-hq/iii, not here. The subdir placement in this PR sidesteps the symptom on the agentmemory side.Summary by CodeRabbit
Release Notes
Documentation
New Features
Tests