feat:Add local personal memory - #808
Conversation
bbcfd23 to
b9a41db
Compare
|
Thanks for splitting the local personal memory work out from #761. Triage: high-caution local-memory enhancement. This is a better review shape than the combined workflow PR, but it still needs a security/privacy pass: local-only storage boundaries, delete semantics, |
b9a41db to
44060c4
Compare
|
Thanks for the review. I updated the PR with a security/privacy pass and kept it scoped to the memory feature. What changed:
I also adjusted the default behavior to avoid changing existing installs:
Validation:
I squashed it back to one commit: |
cfa69f3 to
86dcd5f
Compare
0253e9d to
51f63b9
Compare
|
Status update after a full review pass — being straight with you about where this stands. Review outcome: security-clean (no network/exec surface, storage-path validation works as described, env access through the safe helpers), the Makefile/lint wiring is correct, and the privacy rework you did after #761 addressed the concerns from that thread. Two mechanical items would still be needed: a rebase (main has moved substantially — the PR currently conflicts) and the three raw Decision: deferring this past the v0.9.1 release. The honest reason is direction, not quality: a personal-memory subsystem is a significant permanent surface (a 9-mode MCP tool, a new storage location, sync/promote semantics across scopes), and whether cbm should grow a notes store alongside its knowledge-graph core — versus leaving that to the agents' native memory systems — is a product question we want to settle deliberately, not under release pressure. No action needed from you right now; specifically NOT asking for the rebase yet, so you don't invest in churn while the concept decision is pending. We'll come back to this PR after 0.9.1 ships with a clear yes/no/reduced-scope answer. Sorry for the long runway on this one — the split from #761 and the privacy pass were genuinely good work, and this deferral is not a soft rejection. |
|
No problem, thanks for taking a look anyway |
51f63b9 to
2e395d6
Compare
|
Reviewed in full. The engineering here is careful and one design decision is genuinely better than what we do elsewhere — but there is a direction question for the maintainer and three things bundled in that need to come out regardless. The privacy engineering is a higher bar than most of our own tools started at. Directories created 0700 with chmod-correction, redaction-by-default with The strongest point in its favour, and I want to be specific: storing memory in a separate Also worth noting: no graph schema change at all — zero new node labels, zero new edge types. The code graph is untouched. Three things need to come out of this PR, independent of the direction call.
The direction question, which is the maintainer's. This is not a code-knowledge-graph feature — it stores user- and agent-authored notes rather than extracted structure, and adds no graph capability. It is an adjacent product riding in the same process. It is thematically close to the project's name and to the existing Two smaller things for whenever it resumes:
Please hold off reworking until there is a direction answer; between the rebase and the unbundling, that is real work and I would rather it be spent once. |
|
I owe you a real answer rather than a third deferral, so here it is: this needs a Discussion before it can be a PR decision, and I'd like you to open it. Not because the work is weak — the opposite. The security review came back clean, the out-of-repo storage design is genuinely better than what we do today, and you've already absorbed two rounds of narrowing without complaint. The problem is that I can't responsibly decide this from inside a PR thread. What you're proposing isn't a bug fix or an extension of something existing. It's a 16th first-class MCP tool and, with it, a second axis for the product: cbm today answers questions about code it has indexed, and So: please open a Discussion (Discussions are enabled on the repo), and I'll engage there properly and point others to it. What I'd most want it to surface:
One thing from your PR I want on the record in that Discussion, because it's the most valuable thing to come out of this and it's yours: storing notes outside the project database is architecturally better than what we do now. I went and confirmed it while reviewing something else — ADRs survive a reindex only because there is bespoke capture-and-restore machinery for them ( Three things need to come out regardless of which way it goes, so they don't confuse the conversation:
Also minor, for whenever: the storage-boundary check is a string prefix rather than a canonicalised path, so a symlinked I'm leaving this open and linked to the Discussion. Thank you for your patience — being deferred twice and asked to narrow twice is genuinely frustrating, and you've handled it with more grace than the situation earned. |
|
@alecuba16 — status change and a path forward, so this stops sitting in limbo. The PR is converted to draft, and the direction question now has the community forum it needed: Discussion #1878. The Discussion the 20 August comment asked for never materialised, so we opened it ourselves rather than let the idea rot inside a stale branch — it references this PR as the concrete proposal, credits your privacy engineering explicitly (default-off, 0700 dirs, redaction-by-default with leak tests, no network — that work is genuinely good and is said so there), and lays out the real alternatives, including the external-companion shape from Discussion #1835. To be equally plain about this branch: whatever the Discussion concludes, the bundled fork-release workflow and the unrelated Please weigh in on the Discussion — it is your idea, and the strongest case for it is yours to make. The draft state means "parked pending direction", not "rejected". |
c357104 to
dd19404
Compare
… tests Raw ANSI file operations break on non-ASCII Windows paths. The three remove() calls on memory_db and config_db paths in the manage_memory tests are replaced with cbm_unlink, and rmdir() with cbm_rmdir, matching the pattern used everywhere else in the test suite. Addresses reviewer feedback on PR DeusData#808.
|
Update: the branch is now rebased onto current main and shows as MERGEABLE. The three raw I have also posted a response on Discussion #1878 answering the three questions raised there. The PR will stay in draft until that discussion converges, as requested. |
… tests Raw ANSI file operations break on non-ASCII Windows paths. The three remove() calls on memory_db and config_db paths in the manage_memory tests are replaced with cbm_unlink, and rmdir() with cbm_rmdir, matching the pattern used everywhere else in the test suite. Addresses reviewer feedback on PR DeusData#808. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
e435a51 to
8ab1ba2
Compare
Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
After rebasing onto main, main added check_index_coverage as the 15th tool. Our manage_memory makes 16 total. Update all tool count references, add manage_memory to TOOL_ANNOTATIONS array, and fix test expectations. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
… tests Raw ANSI file operations break on non-ASCII Windows paths. The three remove() calls on memory_db and config_db paths in the manage_memory tests are replaced with cbm_unlink, and rmdir() with cbm_rmdir, matching the pattern used everywhere else in the test suite. Addresses reviewer feedback on PR DeusData#808. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
This file was a local working note that was accidentally staged. It is not part of the personal memory feature. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
Per reviewer feedback on PR DeusData#808, remove three pieces of dead/unrelated surface that were flagged as required regardless of the discussion outcome on Discussion DeusData#1878: 1. auto_update config flag — unrelated feature about the release-check network call, belongs in its own PR if pursued. 2. memory_default_scope config key — stored, printed, and returned in settings JSON but never read by any code path that changes behavior. A knob that does nothing. 3. sync mode — a manage_memory mode whose only function is to return sync_disabled as an error. Shipping a mode that cannot succeed is worse than not shipping it. Also fixes tool count references from 16 to 17 (upstream added two tools since the last rebase: get_file_outline and compare_graphs, bringing the upstream total to 16; our manage_memory makes 17). Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
8ab1ba2 to
c19373c
Compare
|
Following up on the three items flagged as required regardless of the Discussion #1878 outcome. All three are now removed from this branch, and the branch is rebased onto current main (mergeable). 1. 2. 3. Dead surface — removed.
Tool count updated to 17. Upstream added two tools since the last rebase ( The branch is build-clean and the test suite passes (7783 passed, 1 pre-existing failure on main, 7 skipped). The PR stays in draft pending Discussion #1878. |
The real binary exposes 18 tools via tools/list. Upstream main has 17 (get_file_outline and compare_graphs were added since the last rebase), plus our manage_memory makes 18. Updated all references in README, docs/index.html, docs/llms.txt, CLI skill text, tests, and smoke-invariants. Verified via MCP stdio: tools/list returns 18 tools, smoke-invariants passes the tools-list check with 18 tools. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
|
Correction to the tool count in the previous comment: the real binary exposes 18 tools via Verified end-to-end via the MCP stdio protocol against an indexed project with
|
The reviewer on PR DeusData#808 flagged twice that the repo-boundary check was lexical (string prefix) with no realpath canonicalization, so a symlinked memory_dir pointing into the repo would bypass it. Low severity since the input comes from local config or env, not from MCP arguments, but cheap to harden. Use cbm_canonical_path (realpath on POSIX, GetFinalPathNameByHandleW on Windows) to resolve both the repo root and memory_dir before the prefix comparison. Falls back to the lexical check when canonicalization fails (e.g. path does not exist yet). Verified via MCP stdio: a symlinked CBM_MEMORY_DIR pointing into the repo is now correctly rejected with storage_boundary=memory_dir_inside_repo. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
|
One more item addressed from the review: the storage-boundary check is now hardened with realpath canonicalization ( Verified via MCP stdio against a test repo with a symlinked memory_dir: the boundary check catches the symlink and rejects storage, where the old lexical check would have let it through. |
On macOS, /tmp is a symlink to /private/tmp. When the memory dir (e.g. /tmp/foo/.cbm-memory) does not exist yet, realpath fails and the boundary check fell back to the lexical path, which used /tmp/... while the repo root was canonicalized to /private/tmp/... The prefix comparison failed, allowing memory storage inside the repo. Fix: resolve the parent directory (which exists) and append the tail, so both paths use the same canonical prefix. Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
89f1edc to
53a97f0
Compare
Signed-off-by: Alejandro Blanco-M <alecuba16@gmail.com>
This pull request adds a local personal memory system for codebase notes, decisions, and learnings. It is intentionally separated from the fork release workflow work so this PR only contains the personal-memory feature.
This replaces the closed broad PR #761 with the workflow option removed, and addresses the security review note from #761 (comment).
Personal memory
The main addition is a per-repo personal memory that lives outside the repository, in the user's local data directory. The goal is to let an agent or developer store notes, decisions, and learnings about a codebase without writing files into the source repository or pushing that context upstream.
A new MCP tool called
manage_memoryis exposed with these modes:getupdatesectionssettingsbootstrapdeletelistpromotesyncmanage_adralso gains an optionalscopeparameter so it can target either project-scoped ADRs, which remains the default for compatibility, or personal memory.The memory directory, default scope, and enablement are configurable. A new platform helper,
cbm_resolve_memory_dir, resolves the correct local user-data path for each operating system.Security and local-only behavior
After the review comment on #761, I added a focused security hardening pass in commit
5f0f4b6:0700permissions on POSIX systems.0700when opened for writes.manage_memory(settings)redacts local filesystem paths by default.reveal_paths=true.deleteis now idempotent, returnsnot_foundwhen appropriate, and no longer creates a DB just to delete from it.promoteandsyncare local copy operations only. Responses now explicitly reportlocal_onlyandnetwork_sync=disabled.Nothing in this feature uploads personal memory to the repository, remotes, package registries, or external services. Storage is local SQLite under the resolved personal memory directory.
Other changes
manage_memoryand personal scope inmanage_adr.cbm_setenv/cbm_unsetenvhelper was added for Windows-compatible tests.Validation
mainat09148ab.make -f Makefile.cbm testpasses locally:5766 passed.