feat(integrations): add Pi memory package - #1492
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c88864c242
ℹ️ 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".
| const now = new Date(); | ||
| const openedWith = userTurns[0]?.text ?? "Pi session"; | ||
| const recent = params.turns.slice(-8); | ||
| const title = params.title?.trim() || `Pi session ${now.toISOString().slice(0, 19).replace("T", " ")} — ${clip(openedWith, 48)}`; |
There was a problem hiding this comment.
When /bm-capture is invoked without a title, or whenever autoCapture runs, the title includes the current timestamp, so captures more than a second apart target different permalinks. Consequently, --overwrite never updates the previous checkpoint and every settled turn can create another permanent note for the same session, despite the available stable session ID and the bundled capture contract requiring same-thread recaptures to rewrite one note. Use the session ID to find/reuse the original title or otherwise persist a stable per-session title.
Useful? React with 👍 / 👎.
| for (const row of rows.slice(0, 5)) { | ||
| lines.push(`- ${row.title ?? "(untitled)"} — ${row.permalink ?? row.file_path ?? ""}`.trim()); | ||
| if (row.content) lines.push(` ${row.content.replace(/\s+/g, " ").slice(0, 500)}`); | ||
| } | ||
| lines.push("```", "", "Use these note references when continuing the task."); |
There was a problem hiding this comment.
A recalled note controls title, permalink, and content, but these values are inserted into a fixed triple-backtick fence without escaping or choosing a longer delimiter. For example, a synced note with a title containing a newline followed by ``` can close the fence and place attacker-controlled text outside the region labeled as untrusted reference data; this affects both explicit and automatic recall. Derive a fence longer than every backtick run in the rendered results, as the existing lifecycle recall path does.
Useful? React with 👍 / 👎.
| throw new Error(`basic-memory Pi config has unknown keys: ${unknown.join(", ")}`); | ||
| } | ||
|
|
||
| const transport = data.transport === "mcp" ? "mcp" : "cli"; |
There was a problem hiding this comment.
When a known transport key contains any value other than the exact string "mcp"—for example a typo such as "mpc"—configuration parsing silently selects CLI mode. That bypasses the visible configuration-error path and can execute local CLI operations when the user intended MCP; missing values may default, but present invalid enum values should fail validation. This also conflicts with the repository's fail-fast rule against silent fallback logic.
AGENTS.md reference: AGENTS.md:L132-L133
Useful? React with 👍 / 👎.
c88864c to
b6ed753
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6ed753c9e
ℹ️ 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".
| name: cfg.mcpServerName, | ||
| definition: { | ||
| command: cfg.bmPath, | ||
| args: ["mcp", "--transport", "stdio"], |
There was a problem hiding this comment.
When transport is "mcp" and project or projectId is configured, the registered process receives only bm mcp --transport stdio, so adapter calls that omit their own project argument fall through to the user's global default. I checked basic-memory mcp --help, which documents --project as “Restrict MCP server to single project,” and src/basic_memory/cli/commands/mcp.py:73-83 confirms that this establishes the server constraint. As written, /bm-status can report one project while MCP reads or writes another; pass the configured project constraint when launching the server and explicitly resolve or reject the unsupported projectId case.
AGENTS.md reference: AGENTS.md:L393-L395
Useful? React with 👍 / 👎.
b6ed753 to
b80f2ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80f2ec4ac
ℹ️ 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".
| "version": "0.23.2", | ||
| "type": "module", | ||
| "description": "Basic Memory package for Pi — local-first continuity so you are never starting over", | ||
| "license": "MIT", |
There was a problem hiding this comment.
When this package is published, its npm metadata advertises MIT while the bundled integrations/pi/LICENSE contains the GNU AGPL v3 text. Because LICENSE is included in the package tarball, consumers receive conflicting licensing terms and cannot safely rely on the advertised grant; either declare the package as AGPL-3.0-or-later or bundle the intended MIT license.
Useful? React with 👍 / 👎.
| const data = raw && typeof raw === "object" && !Array.isArray(raw) | ||
| ? raw as Record<string, unknown> | ||
| : {}; |
There was a problem hiding this comment.
The new transport check is strict, but other present known keys still silently become defaults: for example, {"project": 123} produces project: undefined, and a non-object JSON root also becomes the complete default configuration. In those cases captures and recalls can run against the user's global default project rather than reporting the malformed project configuration, so distinguish missing values from invalid values and reject the latter.
AGENTS.md reference: AGENTS.md:L132-L133
Useful? React with 👍 / 👎.
| const stableSessionTitle = params.sessionId | ||
| ? `Pi session ${clip(params.sessionId, 24)} — ${clip(openedWith, 48)}` | ||
| : `Pi session ${now.toISOString().slice(0, 19).replace("T", " ")} — ${clip(openedWith, 48)}`; |
There was a problem hiding this comment.
The stable session-ID title added for repeated captures does not distinguish branches within that session. Two Pi branches share the session ID and opening user turn, so automatic captures derive the same title and captureSession writes with --overwrite; capturing branch B therefore replaces branch A's canonical checkpoint with contradictory branch state. Include a stable branch/leaf identity while continuing to reuse the title for recaptures of the same branch.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
b80f2ec to
ae517fb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae517fb5d7
ℹ️ 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".
ae517fb to
5f1427c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f1427ceaf
ℹ️ 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".
| pack: | ||
| npm pack --dry-run | ||
|
|
||
| check: install fetch-skills check-types test pack |
There was a problem hiding this comment.
This check refreshes the copied skills in-place and then tests and packs the modified working tree without checking for a diff. If a change updates a canonical top-level skill but omits the corresponding Pi copy, just package-check-pi and the new CI job still pass while the committed package remains stale for checkout-based installs; fail the check when fetch-skills leaves generated files dirty.
AGENTS.md reference: AGENTS.md:L72-L72
Useful? React with 👍 / 👎.
| - Bundled Basic Memory skills in `skills/`. | ||
| - Skill integration docs in `SKILLS.md`. | ||
| - User README in `README.md`. | ||
| - MIT license. |
There was a problem hiding this comment.
Fresh evidence after the package metadata was corrected is that this shipping checklist still identifies the bundled license as MIT, while both package.json and integrations/pi/LICENSE now declare AGPL-3.0-or-later. Maintainers or release reviewers following this checklist can therefore communicate the wrong licensing terms; update the checklist to match the shipped package.
Useful? React with 👍 / 👎.
5f1427c to
939a360
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 939a3602fa
ℹ️ 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".
132fcb3 to
9e477d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e477d104b
ℹ️ 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".
|
|
||
| const now = new Date(); | ||
| const openedWith = userTurns[0]?.text ?? "Pi session"; | ||
| const recent = params.turns.slice(-8); |
There was a problem hiding this comment.
When auto-capture is enabled—or /bm-capture is invoked repeatedly without a custom title—each capture overwrites the stable session note, but this slice retains only the last eight user/assistant messages. A decision or blocker included in an earlier checkpoint is therefore deleted from the canonical note as soon as enough later turns occur, and subsequent Basic Memory recall cannot recover it. Preserve accumulated durable facts or synthesize a continuing summary instead of replacing the checkpoint with a moving window.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
9e477d1 to
dadf38a
Compare
Signed-off-by: phernandez <paul@basicmachines.co>
dadf38a to
cba6660
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cba6660f29
ℹ️ 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".
| }); | ||
| }); | ||
|
|
||
| child.stdin.end(options.stdin ?? ""); |
There was a problem hiding this comment.
When a large capture is still being written and bm exits or is aborted before consuming stdin—for example, a hung subprocess reaches the timeout—child.stdin emits EPIPE. Because only the child process has an error listener, Node treats the stream error as unhandled and terminates the entire Pi process instead of surfacing the promised capture failure; attach an error handler to stdin and settle the command exactly once.
AGENTS.md reference: AGENTS.md:L130-L133
Useful? React with 👍 / 👎.
| `- Opening request: ${clip(openedWith, 300)}`, | ||
| "", | ||
| "## Thread to date", | ||
| ...thread.map((turn) => `- **${turn.role}:** ${clip(turn.text, 220)}`), |
There was a problem hiding this comment.
Fresh evidence after the earlier eight-turn finding is that the full-thread change still applies clip(turn.text, 220) to every individual message. If a decision, blocker, command result, or next step occurs after character 220 of an ordinary assistant response, it is omitted from every checkpoint and the stable overwrite removes any opportunity to recover it through recall; synthesize the durable state or retain complete turn content rather than storing only each turn's prefix.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
| handler: async (_args, ctx) => { | ||
| const lines = [ |
There was a problem hiding this comment.
After configuration parsing fails, session_start replaces cfg with defaults while retaining configError, but this status handler neither checks nor displays that error. A later /bm-status therefore reports CLI/default-project settings as effective even though capture and recall are disabled and reject every call, making the primary diagnostic command misleading; report the stored configuration error instead of the fallback settings.
AGENTS.md reference: AGENTS.md:L132-L133
Useful? React with 👍 / 👎.
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888a4b0729
ℹ️ 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".
| } | ||
|
|
||
| pi.on("session_start", async (_event, ctx) => { | ||
| await disposeCurrentMcp(); |
There was a problem hiding this comment.
When the previous MCP registration's dispose() rejects—for example after an adapter or server shutdown failure—this await aborts session_start before recall state is reset or the new workspace configuration is loaded. The fresh evidence after the earlier cleanup change is that the disposal await remains outside the startup try; subsequent bm_capture or bm_recall calls can therefore use the preceding session's cfg and route canonical notes to its project. Surface the cleanup error but continue loading the new session configuration, such as via a guarded cleanup or finally.
AGENTS.md reference: AGENTS.md:L130-L131
Useful? React with 👍 / 👎.
1 participant
Why
Pi users should be able to carry Basic Memory continuity across sessions without starting over: capture the current working thread, open a fresh Pi session, and recover the decision, rationale, blocker, and next step from a real Basic Memory note.
This also proves the transport strategy from #1488: ship one Pi package with shared continuity semantics while supporting both direct Basic Memory CLI access and MCP access through the existing
pi-mcp-adapter.What Changed
integrations/pi/published as@basicmemory/pi-basic-memory./bm-status/bm-recall/bm-capturebm_recallbm_capture.pi/basic-memory.json, including CLI/MCP mode, explicit project/project-id routing, capture folder, recall timeframe, and opt-in automation settings.basic-memory-pi, that uses the package'sbm_recallandbm_capturetools.memory-notesmemory-capturememory-continuememory-tasksscripts/fetch-skills.tsandskill-references/manifest.jsonso the package refreshes bundled references from the monorepo source.docs/PI_MEMORY_*.md.just package-check-pijust package-checkandagent-harness-checkscripts/update_versions.pyImplementation Details
The Pi integration is intentionally thin. Basic Memory owns storage, search, graph operations, routing, and authentication; the Pi package owns Pi lifecycle integration, explicit capture/recall commands, and packaging.
CLI mode is the default and shells out to
bm tool ...with bounded subprocess timeouts. MCP mode does not implement a new MCP host; it registers Basic Memory withpi-mcp-adapterthrough the adapter's public runtime registration event. If the adapter is missing, the package reports a visible warning and leaves Pi usable.Automatic recall and capture default off. Users can start with explicit
/bm-recalland/bm-capturebefore opting into automation. Recalled notes are fenced as reference data, not instructions, and captures are synthesized checkpoints rather than raw transcript dumps.Testing
Passed:
That runs:
npm ci --ignore-scripts npm run fetch-skills npm run check-types npm test npm pack --dry-runAlso passed:
Manual isolated E2E evidence is recorded in
docs/PI_MEMORY_E2E_RESULTS.md:All E2E runs used temporary
BASIC_MEMORY_HOME,BASIC_MEMORY_CONFIG_DIR, Pi session directories, and throwaway Basic Memory projects.Risks / Follow-ups
transport: "mcp"; MCP mode currently exposes Basic Memory to the model viapi-mcp-adapter.