[AI-1134] kcap mcp memory — team memory MCP server + persisted machine id#239
Open
alexeyzimarev wants to merge 3 commits into
Open
[AI-1134] kcap mcp memory — team memory MCP server + persisted machine id#239alexeyzimarev wants to merge 3 commits into
alexeyzimarev wants to merge 3 commits into
Conversation
PR Summary by QodoAdd MCP memory server and persisted machine id for machine-scoped memories
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used 1. Repo-scoped save can globalize
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ["content"] = Req("content"), | ||
| ["kind"] = Req("kind"), | ||
| ["team"] = args?["team"]?.GetValue<string>(), | ||
| ["repo_hash"] = global ? null : cwdRepoHash, |
There was a problem hiding this comment.
1. Repo-scoped save can globalize 🐞 Bug ≡ Correctness
save_memory defaults to repo-scoped, but if repo detection fails (cwdRepoHash is null) BuildSaveBody sends repo_hash: null even when global is false, which can unintentionally create a global (repo-independent) memory. This can happen in non-git directories or when the remote URL can’t be parsed into owner/repo, violating the tool’s documented default scoping.
Agent Prompt
## Issue description
`save_memory` is documented as repo-scoped by default, but the request builder currently falls back to `repo_hash = null` whenever `cwdRepoHash` is unavailable. If the server interprets `repo_hash: null` as global, this broadens scope unintentionally.
## Issue Context
Repo hash resolution is best-effort and can legitimately return null (non-git cwd, missing/unknown remote, parse failures).
## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[99-110]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[228-247]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[342-353]
## Suggested fix
- In `BuildSaveBody`, if `global == false` and `cwdRepoHash == null`, throw `ArgumentException` with an actionable message (e.g. "Unable to resolve repo for cwd; run inside a git repo or pass global:true").
- (Optional) Consider adding an explicit tool argument like `repo_hash` for callers that can provide it, but only if that matches product intent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ["kind"] = Req("kind"), | ||
| ["team"] = args?["team"]?.GetValue<string>(), | ||
| ["repo_hash"] = global ? null : cwdRepoHash, | ||
| ["machine_tag"] = machineSpecific ? machineId : null, |
There was a problem hiding this comment.
2. Machine_specific may be dropped 🐞 Bug ≡ Correctness
If machine id resolution fails, ResolveMachineIdAsync returns null and BuildSaveBody will send machine_tag: null even when machine_specific: true, silently saving a non-machine-specific memory. This breaks the expected isolation of machine-specific memories and can cause them to surface on other machines.
Agent Prompt
## Issue description
When `machine_specific` is requested, the server call must include a non-null machine tag. Today, failures in machine-id load/create are swallowed and `machine_tag` becomes null.
## Issue Context
`ResolveMachineIdAsync` catches all exceptions and returns null; `BuildSaveBody` uses `machineId` without enforcing non-null when `machine_specific` is set.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[112-114]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[228-247]
## Suggested fix
- In `BuildSaveBody`, if `machine_specific == true` and `machineId == null`, throw `ArgumentException` ("machine_specific requires a persisted machine id; fix config permissions or retry").
- (Optional) In `ResolveMachineIdAsync`, log a one-line stderr warning when it fails so the behavior isn’t silent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+15
| public static async Task<string> GetOrCreateAsync() { | ||
| var config = await AppConfig.LoadProfileConfig(); | ||
| if (!string.IsNullOrWhiteSpace(config.MachineId)) return config.MachineId; | ||
|
|
||
| var id = Generate(); | ||
| await AppConfig.SaveProfileConfig(config with { MachineId = id }); | ||
| return id; |
There was a problem hiding this comment.
3. Unvalidated persisted machine_id 🐞 Bug ☼ Reliability
MachineIdProvider.GetOrCreateAsync trusts any non-whitespace machine_id from config without
validating the expected mach-{12hex} format, so a corrupted/manual value will be propagated into
requests. This can lead to server-side validation failures or incorrect machine scoping behavior.
Agent Prompt
## Issue description
The code assumes `machine_id` has a stable format (and tests enforce generation format), but `GetOrCreateAsync` returns any non-empty persisted value as-is.
## Issue Context
`MachineIdProvider.Generate()` produces `mach-` + 12 hex characters, and unit tests assert this format; persisted config values are not checked.
## Fix Focus Areas
- src/Capacitor.Cli.Core/MachineIdProvider.cs[5-16]
- test/Capacitor.Cli.Tests.Unit/MachineIdTests.cs[7-13]
## Suggested fix
- Add a small validator (regex or manual check) for `^mach-[0-9a-f]{12}$`.
- In `GetOrCreateAsync`, if `config.MachineId` is present but invalid, generate a new id and persist it (or surface a clear error if you prefer not to auto-heal).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the CLI side of Agent Memories (AI-1134) — server-side shared memories for coding agents, scoped user/team/org with repo + machine context.
What's in here
kcap mcp memory— stdio MCP server (copy of theMcpSessionsServerskeleton) with six tools:search_memories,get_memory,save_memory,update_memory,rescope_memory,archive_memory. Tool descriptions steer agents to search before saving and preferupdate_memoryon a reported near-duplicate.repo_hash,machine_tag, …) matching the server's globalSnakeCaseLowerJSON policy; responses pass through as raw text. Bodies built withJsonObject— noMcpJsonContextadditions, AOT publish clean (no new IL3050/IL2026).machine_id(mach-{12hex}) on the top-levelProfileConfig, generated once byMachineIdProvider; stamps machine-specific memories so they never surface on other machines.kcap/.mcp.json(Claude, withcwd) andkcap/.codex-mcp.json(nocwd— matches that file's precedent); built-in help (help-usage.txt,help-mcp.txt) and both READMEs updated.Tests
Capacitor.Cli.Tests.Unit: 2077/2077 green (9 new: machine-id format/round-trip/back-compat + URL/body builders incl. snake_case,global,machine_specificrules).Notes
alexeyzimarev/ai-1134-agent-memories(bump the submodule pointer toorigin/mainafter this merges).kcap-memoryinto thekcap setup --codexallowlist + install docs.🤖 Generated with Claude Code