memory: memsearch parity + reindex-after-sync, pinned sync identity - #166
Conversation
…ontract, pinned knowledge-sync identity, rem search collection ai
Reviewer's GuideThis PR makes memsearch a machine-local derived backend over canonical vault markdown, with parity installation and coordinated refreshes after capture and sync; it also hardens unattended Git identity resolution and documents the separate dream-pass ownership model while defaulting rem search to collection ai. Sequence diagram for coordinated memsearch refreshessequenceDiagram
participant Capture as basic_memory capture
participant Sync as knowledge-sync
participant Lock as reindex.lock
participant Search as memsearch
participant Vault as Canonical vault markdown
alt session digest written
Capture->>Lock: flock(LOCK_EX | LOCK_NB)
Lock-->>Capture: lock acquired or skip
Capture->>Search: index(Vault, --collection ai)
Search-->>Capture: refresh result
else sync completes
Sync->>Lock: syscall.Flock(LOCK_EX | LOCK_NB)
Lock-->>Sync: lock acquired or skip
Sync->>Search: index(Vault, --collection ai)
Search-->>Sync: refresh result or timeout
end
Sequence diagram for pinned knowledge-sync commit identitysequenceDiagram
participant Sync as knowledge-sync
participant Env as Environment
participant GitConfig as Git config
participant Git as git
Sync->>Env: resolveIdentity(repo)
Env-->>Sync: KNOWLEDGE_GIT_AUTHOR_* or GIT_AUTHOR_*
alt identity incomplete
Sync->>GitConfig: git config --get user.name/user.email
GitConfig-->>Sync: configured values or empty
end
alt name and email resolved
Sync->>Git: commit -c user.name -c user.email -m message
Git-->>Sync: commit result
else identity unresolved
Sync-->>Sync: refuse commit
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="memory/tools/knowledge-sync/main.go" line_range="66" />
<code_context>
msg := "sync knowledge " + time.Now().UTC().Format("2006-01-02T15:04:05Z")
- if out, err := git(repo, "commit", "-m", msg); err != nil {
+ commitArgs := []string{"-c", "user.name=" + name, "-c", "user.email=" + email, "commit", "-m", msg}
+ if out, err := git(repo, commitArgs...); err != nil {
// Nothing to commit after add is harmless; anything else is not.
if !strings.Contains(out, "nothing to commit") && !strings.Contains(out, "no changes added") {
</code_context>
<issue_to_address>
**issue (bug_risk):** The explicit `-c user.name` and `-c user.email` values do not override Git's `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL` or committer environment variables, so a dedicated `KNOWLEDGE_GIT_AUTHOR_*` pin can still produce a commit with a different author or committer identity.
**Triggers:** When LaunchAgent or interactive environments still contain `GIT_AUTHOR_*` or `GIT_COMMITTER_*` values alongside `KNOWLEDGE_GIT_AUTHOR_*`.
**Suggested fix:** Clear the relevant Git identity environment variables for the commit or pass the resolved identity explicitly with `--author` and controlled committer environment variables.
```suggestion
for _, key := range []string{"GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL", "GIT_COMMITTER_NAME", "GIT_COMMITTER_EMAIL"} {
_ = os.Unsetenv(key)
}
if out, err := git(repo, commitArgs...); err != nil {
```
</issue_to_address>
### Comment 2
<location path="memory/tools/knowledge-sync/main.go" line_range="185-191" />
<code_context>
+ }
+ defer func() { _ = syscall.Flock(int(rlock.Fd()), syscall.LOCK_UN) }()
+
+ knowledge := getenv("KNOWLEDGE_DIR", repo)
+ collection := getenv("MEMSEARCH_COLLECTION", "ai")
+ ctx, cancel := context.WithTimeout(context.Background(), reindexTimeout())
+ defer cancel()
+
+ // Incremental by default (only files changed by the merge are re-embedded).
+ cmd := exec.CommandContext(ctx, bin, "index", knowledge, "--collection", collection)
+ out, err := cmd.CombinedOutput()
+ if ctx.Err() == context.DeadlineExceeded {
</code_context>
<issue_to_address>
**issue (broader_impact):** The refresh triggers honor `MEMSEARCH_COLLECTION`, so setting it to a non-`ai` value makes them update a different collection while the documented shared contract and default `rem search` continue using `ai`; the canonical collection then becomes stale.
**Triggers:** When `MEMSEARCH_COLLECTION` is set to a custom collection in the sync or capture environment.
**Suggested fix:** Use the fixed `ai` collection for both freshness triggers, or document and apply the override consistently to indexing and search.
</issue_to_address>
### Comment 3
<location path="memory/lib/sync.py" line_range="260-266" />
<code_context>
+ print("memsearch reindex: memsearch not installed, skipped")
+ return
+
+ state_dir = paths["memsearch_home"]
+ try:
+ state_dir.mkdir(parents=True, exist_ok=True)
+ except OSError:
+ pass
+ lock_path = state_dir / "reindex.lock"
+ lock = open(lock_path, "w")
+ try:
+ fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB)
</code_context>
<issue_to_address>
**issue (bug_risk):** If creating the memsearch state directory fails, the exception is swallowed and the function then calls `open(lock_path, "w")`, which raises `OSError` and aborts the caller instead of remaining best-effort.
**Triggers:** When `MEMSEARCH_HOME` is missing, inaccessible, read-only, or otherwise cannot be created.
**Suggested fix:** Return after the failed `mkdir`, and handle lock-file open failures as a skipped refresh.
```suggestion
state_dir = paths["memsearch_home"]
try:
state_dir.mkdir(parents=True, exist_ok=True)
except OSError:
print("memsearch reindex: unable to create state directory, skipped")
return
lock_path = state_dir / "reindex.lock"
try:
lock = open(lock_path, "w")
except OSError:
print("memsearch reindex: unable to open lock file, skipped")
return
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and if the pinned identity or reset behavior is wrong, unattended sync can create commits attributed to the wrong person, and those pushed history records are not repaired by reverting this change. The memsearch index itself is disposable and rebuildable, but correcting already-published commit attribution would require history repair.
Blocking findings: memory/tools/knowledge-sync/main.go:66, memory/tools/knowledge-sync/main.go:191, memory/lib/sync.py:266
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a07355373
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| commitArgs := []string{"-c", "user.name=" + name, "-c", "user.email=" + email, "commit", "-m", msg} | ||
| if out, err := git(repo, commitArgs...); err != nil { |
There was a problem hiding this comment.
When KNOWLEDGE_GIT_AUTHOR_* and GIT_AUTHOR_* are both present, resolveIdentity selects the dedicated values, but the inherited GIT_AUTHOR_* variables take precedence over these user.* configuration overrides during git commit. For example, Git records the inherited author while using the resolved value only for the committer, defeating the documented preference order and pinned-author guarantee. Set explicit author and committer environment variables on the commit command, or remove conflicting inherited variables.
Useful? React with 👍 / 👎.
| cmd = ["memsearch", "index", str(paths["vault_dir"]), "--collection", paths["collection"]] | ||
| result = subprocess.run(cmd, capture_output=True, text=True) |
There was a problem hiding this comment.
When SESSIONS_DIR, NOTES_DIR, or PROFILE_DIR points outside KNOWLEDGE_DIR, the sync still reads and writes those configured locations, but the new command indexes only vault_dir. This regresses the previous implementation, which explicitly indexed each resolved directory, so a successful sync can leave the changed external files absent from search results. Either retain the resolved paths or require and validate that they reside under the vault root.
Useful? React with 👍 / 👎.
| state_dir = paths["memsearch_home"] | ||
| try: | ||
| state_dir.mkdir(parents=True, exist_ok=True) | ||
| except OSError: | ||
| pass | ||
| lock_path = state_dir / "reindex.lock" | ||
| lock = open(lock_path, "w") |
There was a problem hiding this comment.
When MEMSEARCH_HOME is unwritable, is an existing regular file, or otherwise cannot host reindex.lock, mkdir errors are ignored and this open raises an uncaught OSError. Because main calls this after modifying the canonical memory files, a supposedly best-effort reindex makes the overall sync exit with failure after partial success. Catch lock creation errors and report/skip them just like a missing binary or held lock.
Useful? React with 👍 / 👎.
…i, keep reindex best-effort on state-dir failure
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…e/sessions scope so sync and capture triggers mutually exclude
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Makes
memsearcha first-class, equal-citizen search backend on every machine, with derived per-machine indexes and automatic refresh, plus the audit's sync/consolidation fixes (report sections 2, 5, 7, 8). No per-machine index is ever synced or canonical; the vault markdown is the only source of truth.Changes (all within the memory tool surface)
R2 - reindex-after-sync (
knowledge-sync)After a successful pull/merge/push,
knowledge-synctriggers a bounded, best-effort refresh of the derived index (memsearch index <vault> --collection ai). It never blocks the sync (git work is already done), skips when a concurrent refresh holds the shared lock, and is a no-op whenmemsearchis absent. This plus reindex-after-capture replace a reindex cron. Timeout viaMEMSEARCH_REINDEX_TIMEOUT_SECONDS(default 300s).Shared reindex contract (
memory/tools/memsearch/README.md)Both freshness triggers are idempotent, target collection
ai, index the whole vault, and coordinate through one non-blocking OS advisory lock at~/.memsearch/reindex.lockso they never overlap.sync.py's post-sync reindex now uses the same canonical whole-vault scope and lock.Multi-machine parity (
memory/tools/memsearch/install-parity.sh+ README)One idempotent script brings any machine to full parity:
uv tool install 'memsearch[onnx]', configprovider=onnx+collection=ai, index derived from the canonical markdown, and--rebuildfor a documented full rebuild from zero. Documents that the index is disposable and never synced.R4 - two-dreams split: documented one owner per input
Chose the smaller correct change (documentation, not code): Go
rem dreamownsai/candidate promotion; Pythonbasic_memory dreamownssessions/digest review. They consume different inputs and neither feeds the other. Pointing Gorem dreamatsessions/would have added a third consumer of that input and increased the duplication R4 asks to remove; documenting the split respects the ownership boundary (basic_memory.pyis owned by another change).R6 - pinned knowledge-sync identity
Root cause: there is no persistent git identity on this host (no
~/.gitconfig, no repo-local config); the vault relies onGIT_AUTHOR_*env from interactive agent sessions. LaunchAgent runs lack that env, so git synthesized a host-detectedconscience@<hostname>identity - the "configured automatically based on your username and hostname" warning andEX_CONFIGstatus. Fix: commits now pass explicit-c user.name/-c user.email, resolved fromKNOWLEDGE_GIT_AUTHOR_NAME/EMAIL->GIT_AUTHOR_*->git config, and refuse to commit rather than allow a host-detected fallback. The warning is prevented by construction.rem searchnow targets collectionaiunless overridden (REM_COLLECTION).Verification
go test ./...(workspace,rem,knowledge-syncmodules),go vet,gofmt, Pythonunittest, andshellcheckall clean. New focused tests: identity resolution precedence + host-detected refusal;rem searchcollection injection;sync.pyreindex scope + shared lock.gamrevinu@m1, macOS 26.4.1): installedmemsearch[onnx](0.4.19), wrote config (provider=onnx,collection=ai), full rebuild from zero -> 4418 chunks. Real searches verified: "preferred relocation locations for job search" ->profile/USER.mdJob search (score 1.0000).Notes for deploy
knowledge-sync(viadotagents sync), pin the identity - repo-localgit -C ~/Workspace/knowledge config user.name/user.emailorKNOWLEDGE_GIT_AUTHOR_*in the LaunchAgent plist - otherwise the tool will refuse to commit rather than mis-author. No launchd/~/.memsearchedits were made on the local Mac as part of this change.Summary by Sourcery
Establish parity and freshness for per-machine memsearch indexes while hardening knowledge-sync identity handling and clarifying memory workflow ownership.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: