fix(mcp): stop move_note reporting false success across boundaries

[phernandez]

Summary

move_note silently performed a same-project move (creating a nested folder) and reported ✅ Note moved successfully when an agent intended a cross-workspace/cross-project move, misleading the agent into thinking the note crossed the boundary. This PR makes the tool honest about the unsupported case. It does not add real cross-project moves (a separate, larger feature needing a destination-project/workspace parameter end-to-end).

Closes #881.

What changed

src/basic_memory/mcp/tools/move_note.py:

• Gap 3 (ordering): moved the cross-boundary guard so it runs after destination_folder is resolved into destination_path. Previously the guard ran while destination_path was still "" for destination_folder callers, so boundary-shaped folders bypassed detection entirely.
• Gap 2 (outcome validation, the robust backstop): after move_entity returns, compare the normalized requested destination against the resulting result.file_path. If they diverge in a way the caller did not ask for, return a failure (moved: False / # Move Failed - Unexpected Result Location) instead of unconditionally printing ✅ Note moved successfully.
• Gap 1 (broaden detection, conservatively): _detect_cross_project_move_attempt now rejects two clear cross-boundary shapes — a leading path segment equal to a known project name, and the cloud workspace shape <workspace>/projects/<x>/... (interior projects segment). Legitimate same-project moves into new nested folders (including a top-level projects/2025/...) are not flagged.

Testing

Commands run (SQLite, no Docker):

• uv run ruff check . --fix and uv run ruff format . — clean.
• uv run ty check src tests test-int — All checks passed!
• uv run pytest tests/mcp/test_tool_move_note.py test-int/mcp/test_move_note_integration.py -q — 67 passed.
• uv run pytest tests/mcp/ test-int/mcp/ -q — 875 passed, 1 unrelated transient failure in test_delete_note_integration.py (Could not load model BAAI/bge-small-en-v1.5 embedding-model load during the large parallel run); it passes on isolated rerun (9 passed) and touches no move_note code.

New integration tests (test-int/mcp/test_move_note_integration.py):

• test_move_note_workspace_shaped_path_rejected — workspace-shaped destination fails with cross-project error; note stays put; nested path is not created.
• test_move_note_destination_folder_boundary_rejected — the destination_folder bypass (Gap 3) is now detected.
• test_move_note_workspace_shaped_path_rejected_json — JSON output reports moved: False / CROSS_PROJECT_MOVE_NOT_SUPPORTED.
• test_move_note_new_nested_folder_still_succeeds — legitimate same-project nested move still succeeds (false-positive guard).

New unit tests (tests/mcp/test_tool_move_note.py, TestMoveNoteOutcomeValidation):

• Outcome-mismatch backstop (text + JSON). These inject a divergent file_path via monkeypatch on KnowledgeClient.move_entity because the real service stores the requested path verbatim, so divergence cannot occur through the live stack — this is the legitimate "inject a failure" case for a mock.

Risk / validation

• Low risk. Detection is conservative: only a leading project-name segment or the explicit <workspace>/projects/<x> shape is rejected, so normal nested moves are unaffected (verified by existing test_move_note_allows_safe_paths and the new false-positive test). The outcome-validation backstop only fires when the landing path genuinely differs from the request, which never happens for valid same-project moves.

Closes #881

🤖 Generated with Claude Code

[github-actions]

Claude finished @phernandez's task in 2m 15s —— View job

---

Code Review: fix(mcp): stop move_note reporting false success across boundaries

• Read changed files and gather context
• Review code quality and standards
• Review testing and coverage
• Check architecture and design
• Check compatibility and security
• Post final review summary

---

Overall this is a solid, well-motivated fix. The three-gap framing is clear, the comments follow the project's Trigger/Why/Outcome style, and the test coverage is good. One medium-severity false-positive bug and a few smaller issues below.

---

Team Checklist Results

Category	Status
Python 3.12+ type annotations / async patterns	✅
100-character line length	✅
No commented-out code blocks	✅
Literate programming comment style	✅
Windows path compatibility (PureWindowsPath)	✅
Unit tests for new logic	✅
Integration tests for new MCP behaviour	✅
No DB changes / no migration needed	✅
JSON output for all new error paths	✅
No hardcoded secrets / credentials	✅
Input validation for MCP tools	✅
No emoji / unicode in new log/error output	✅ new text avoids emoji
100% test coverage (no gaps in new code)	⚠️ Detection 2 false-positive path not tested

---

Issues

Medium — Detection 2 false positive: interior projects/ segment in any sub-folder

src/basic_memory/mcp/tools/move_note.py:68-72

for index in range(1, len(path_parts) - 1):
    if path_parts[index] == "projects":
        return _format_cross_project_error_response(...)

The loop rejects any path where "projects" appears as a non-leading interior segment, not just the <workspace>/projects/<x>/... cloud layout. A legitimate same-project path like notes/projects/my-project/note.md has "projects" at index 1, so it hits the guard and is incorrectly rejected:

notes/projects/my-project/note.md
 ^0      ^1        ^2        ^3
         "projects" at index 1 → range(1, 3) → MATCH → error ❌

The PR description says the guard is conservative and "a top-level projects/2025/... folder is not flagged", which is true — but only because "projects" sits at index 0 there (below the loop's start). The description does not cover notes/projects/…, which is a plausible real-world folder structure.

The target shape from issue #881 is specifically <workspace>/projects/<x>/... — meaning "projects" at index 1 only, preceded by a workspace segment. Limiting the check to path_parts[1] == "projects" (when len(path_parts) >= 3) would be accurate and still catch the bug:

# Cloud workspace shape: <workspace>/projects/<x>/...
if len(path_parts) >= 3 and path_parts[1] == "projects":
    return _format_cross_project_error_response(
        identifier, destination_path, current_project, path_parts[0]
    )

Fix this →

---

Minor — Missing false-positive test for nested projects/ folder

test-int/mcp/test_move_note_integration.py

test_move_note_new_nested_folder_still_succeeds uses projects/2025/q2/legit-nested-note.md where "projects" is at index 0 — safely below the loop start. It does not exercise the actual false-positive scenario (notes/projects/my-folder/note.md). Adding a second case would lock in the intended behaviour:

# Should also succeed:
move_note("...", destination_path="notes/projects/my-project/note.md")

---

Minor — MOVE_OUTCOME_MISMATCH recovery guidance may be incorrect

src/basic_memory/mcp/tools/move_note.py:908-912

read_note("{identifier}")
write_note("Title", "content", "folder", project="target-project")
delete_note("{identifier}", project="{active_project.name}")

The backstop fires after move_entity returns, meaning the file has already moved to result.file_path. Suggesting delete_note("{identifier}") at the original path could silently fail (or hit a different entity) because the note is now at the diverged location. Pointing the agent to result.file_path for both read_note and delete_note would be more accurate.

---

Positives

• Gap 3 ordering fix is correct. Moving the cross-boundary guard to after destination_folder resolution (line 760+) is the right call. The old position against an empty string was a real bypass path.
• Outcome validation backstop is good defense-in-depth. Even when detection is bypassed, comparing normalized_requested vs normalized_actual via PureWindowsPath catches divergence cleanly and is Windows-safe per CLAUDE.md.
• Comment style. All three added sections follow the Trigger/Why/Outcome literate programming convention required by CLAUDE.md.
• JSON output parity. Every new failure path returns a consistent {"moved": false, "error": "..."} shape.
• Monkeypatch approach in unit tests is appropriate. The PR correctly notes that real divergence cannot occur through the live stack, so injecting it via monkeypatch.setattr on KnowledgeClient.move_entity is the right testing technique.
• Detection 1 (leading segment) is correct. Comparing only the leading segment and excluding the current project avoids false positives entirely for that path.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7d1c63eee

ℹ️ 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".

[chatgpt-codex-connector]

Narrow workspace-shape detection to actual boundary paths

For a normal same-project move into a nested folder named projects, such as archive/projects/moved.md or team/projects/2026/moved.md, this new check rejects the request before move_entity runs even though the path is a valid relative path within the project. The tool otherwise allows arbitrary nested folders and the added integration test only preserves top-level projects/...; matching any interior projects segment makes legitimate organization layouts unusable unless users rename their folder.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 3m 1s —— View job

---

Code Review: fix(mcp): stop move_note reporting false success across boundaries

• Read changed files and gather context
• Review code quality and standards
• Review testing and coverage
• Check architecture and design
• Check compatibility and security
• Post final review summary

---

This is a solid, well-motivated fix. The two-commit structure is clear: commit 1 (a7d1c63) adds the three gaps; commit 2 (20e79d9) tightens Detection 2 in response to reviewer feedback. The overall approach is correct and the test coverage is thorough.

Team Checklist Results

Category	Status
Python 3.12+ type annotations / async patterns	✅
100-character line length	✅
No commented-out code blocks	✅
Literate programming comment style (Trigger/Why/Outcome)	✅
Windows path compatibility (PureWindowsPath)	✅
Unit tests for new logic	✅
Integration tests for new MCP behaviour	✅
JSON output for all new error paths	✅
No DB changes / no migration needed	✅
No hardcoded secrets / credentials	✅
Input validation for MCP tools	✅
No emoji / unicode in new log/error output	✅
Knowledge graph and file-system consistency preserved	✅
100% coverage on new code paths	✅ all new branches exercised
MOVE_OUTCOME_MISMATCH recovery guidance accuracy	⚠️ Minor — see below

---

Medium Bug from Prior Review — Resolved ✅

The loop for index in range(1, len(path_parts) - 1) that caught any interior projects segment has been replaced with the single targeted check path_parts[1] == "projects" in commit 2. This is exactly the fix that was recommended. The accompanying tests cover:

• test_move_note_interior_projects_segment_still_succeeds (integration) — team/2026/projects/alpha/... where "projects" is at index 2 succeeds
• test_detect_cross_project_only_flags_workspace_shape (unit) — workspace-shape rejection, interior acceptance, and top-level projects/2025/... acceptance all verified

---

Remaining Minor — MOVE_OUTCOME_MISMATCH recovery guidance

src/basic_memory/mcp/tools/move_note.py:908-912

read_note("{identifier}")
write_note("Title", "content", "folder", project="target-project")
delete_note("{identifier}", project="{active_project.name}")

The backstop fires after move_entity returns, meaning the file has already been relocated to result.file_path. The agent following this guidance would likely get Note Not Found on both read_note and delete_note because {identifier} no longer points to the original location.

Pointing to result.file_path for both read and delete would be more accurate:

read_note("{result.file_path}")
write_note("Title", "content", "folder", project="target-project")
delete_note("{result.file_path}", project="{active_project.name}")

This is low-severity in practice — the PR correctly notes that real divergence cannot happen through the live stack, so this code path essentially only fires if a future service bug is injected. But since the guidance is what an agent would act on, pointing it at the right location matters.

Fix this →

---

Residual Edge Case — my-folder/projects/x/file.md

Detection 2 (path_parts[1] == "projects") still flags a legitimate same-project path where "projects" happens to be the second segment — e.g. notes/projects/my-project/note.md. This was the specific example in the prior review's bug description. The fix commit addresses the case where "projects" is at index ≥ 2, but index 1 is still caught.

This is the accepted trade-off from the first review's own proposed fix (path_parts[1] == "projects"), and the comment block explicitly names this constraint. Given the very specific organizational choice required to hit this false positive, and the fact that Detection 1 + outcome validation are backstops, this is acceptable as-is — just worth documenting in case a user reports it.

---

Positives

• Gap 3 ordering fix is correct. Moving the guard after destination_folder resolution is the right call — the old position saw an empty string and skipped detection entirely.
• Outcome validation backstop is good defense-in-depth. Even if detection is bypassed, PureWindowsPath normalization + path comparison cleanly surfaces divergence. Windows-safe per CLAUDE.md.
• Comment style throughout. All four added sections (Detection 1, Detection 2, cross-boundary guard, outcome validation) follow the Trigger/Why/Outcome convention.
• JSON parity. Both new failure paths (CROSS_PROJECT_MOVE_NOT_SUPPORTED and MOVE_OUTCOME_MISMATCH) return {"moved": false, "error": "..."} consistently.
• Monkeypatch approach for unit backstop tests is appropriate. The PR correctly justifies injecting divergence via monkeypatch.setattr since real divergence cannot occur through the live stack.
• Second commit addressed reviewer feedback responsively. Replaced the loop with the exact single-check fix that was recommended, added both an integration test and a unit test covering the narrowed behaviour.

---

Overall: The core fix is correct and the test coverage is solid. The MOVE_OUTCOME_MISMATCH guidance is the only unresolved item, and it's low-impact given the backstop's theoretical-only trigger path. Good to merge once the guidance is updated, or carry it as a known minor issue.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20e79d9b03

ℹ️ 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".

[chatgpt-codex-connector]

Report the destination project, not workspace

For workspace-shaped destinations like team-space/projects/alpha/moved.md, this branch formats the error with target_project=path_parts[0], so the guidance says the target project is team-space and even suggests write_note(..., project="team-space"). In the documented <workspace>/projects/<x>/... shape, <x> is the project being targeted, so users following the error message will try to write into the workspace name instead of the actual project (alpha).

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ae705c962

ℹ️ 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".

[chatgpt-codex-connector]

Allow normal folders before projects

For same-project destinations with a second path segment named projects, such as archive/projects/moved.md or the comment's own notes/projects/my-project/note.md, this guard still rejects the move as cross-project even though these are valid relative folders inside the active project. Fresh evidence is that the current code only pins projects to index 1, which still matches common nested organization paths and even treats moved.md as the target project for a three-segment path.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @phernandez's task in 1m 55s —— View job

---

Code Review: fix(mcp): stop move_note reporting false success across boundaries

• Read changed files and gather context
• Review code quality and standards
• Review testing and coverage
• Check architecture and design
• Check compatibility and security
• Post final review summary

---

Three commits, clean progression. Commit 8ae705c (the sync trigger) addresses the Codex finding that the error message named the workspace (path_parts[0]) instead of the actual target project (path_parts[2]) in the <workspace>/projects/<x>/... shape. That's correct and the integration test was updated to assert **Target project:** x. One prior review item remains open.

Team Checklist Results

Category	Status
Python 3.12+ type annotations / async patterns	✅
100-character line length	✅
No commented-out code blocks	✅
Literate programming comment style (Trigger/Why/Outcome)	✅
Windows path compatibility (PureWindowsPath)	✅
Unit tests for new logic	✅
Integration tests for new MCP behaviour	✅
No DB changes / no migration needed	✅
JSON output for all new error paths	✅
No hardcoded secrets / credentials	✅
Input validation for MCP tools	✅
No emoji / unicode in new log/error output	✅
MOVE_OUTCOME_MISMATCH recovery guidance	⚠️ Still points to wrong path — see below

---

Minor (Unresolved from review 2) — MOVE_OUTCOME_MISMATCH recovery guidance uses stale path

src/basic_memory/mcp/tools/move_note.py:913–915

read_note("{identifier}")
write_note("Title", "content", "folder", project="target-project")
delete_note("{identifier}", project="{active_project.name}")

This guidance fires after move_entity returns — meaning the note has already been relocated to result.file_path. An agent following the read_note("{identifier}") and delete_note("{identifier}") calls would get Note Not Found for both because {identifier} no longer exists at its original path; the file is at result.file_path.

Both previous Claude reviews flagged this exact line. Neither of the three commits addressed it. Low-severity in practice because the backstop can only fire if a future service bug causes divergence (which cannot happen through the live stack today), but the guidance is what an agent would act on in that scenario.

Fix: replace {identifier} with {result.file_path} in both the read_note and delete_note lines:

read_note("{result.file_path}")
write_note("Title", "content", "folder", project="target-project")
delete_note("{result.file_path}", project="{active_project.name}")

Fix this →

---

Accepted trade-off — notes/projects/my-project/note.md still false-positives

path_parts[1] == "projects" catches any path where the second segment is literally "projects", which includes notes/projects/my-project/note.md. The code comment at line 67–70 explicitly documents this constraint. The integration test test_move_note_interior_projects_segment_still_succeeds covers team/2026/projects/alpha/... (projects at index 2), but not the index-1 case. This is the accepted trade-off from the prior review's own proposed fix — noting it here for completeness since the Codex review also flagged it again.

---

Positives

• 8ae705c fix is correct. path_parts[2] (the actual project name x in <workspace>/projects/<x>/...) is reported as the target project in the error guidance rather than path_parts[0] (the workspace). The integration test assertion assert "**Target project:** x" in error_message locks this in.
• Gap 3 ordering fix is sound. Moving the cross-boundary guard to after destination_folder resolution closes the bypass where destination_folder callers saw an empty destination_path and slipped through detection.
• Outcome validation backstop is good defense-in-depth. PureWindowsPath normalization + path comparison is Windows-safe per CLAUDE.md and catches divergence regardless of how detection was bypassed.
• Detection 1 (leading segment) is accurate — only checks when the leading segment is a known project name different from the current one.
• Monkeypatch approach for backstop tests is the right technique given that real divergence cannot occur through the live stack.
• Comment style throughout follows the Trigger/Why/Outcome convention from CLAUDE.md.

---

Verdict: Functionally correct and well-tested. The MOVE_OUTCOME_MISMATCH guidance is the sole unresolved item from prior reviews — a one-line fix. Good to merge after that, or carry it as a known minor issue given the backstop's theoretical-only trigger path.

[phernandez]

Follow-up from reviewing the recently closed PRs: I agree with the earlier review finding that the MOVE_OUTCOME_MISMATCH recovery guidance still points at the stale source identifier.

I checked current origin/main, and the text path still says:

read_note("{identifier}")
delete_note("{identifier}", project="{active_project.name}")

This branch fires after move_entity has already returned a divergent result.file_path, so an agent following the guidance would likely read/delete the old location and get Note Not Found. The JSON response already carries the actual file_path, and the text guidance should do the same:

read_note("{result.file_path}")
delete_note("{result.file_path}", project="{active_project.name}")

Low probability because the backstop only fires on an unexpected service divergence, but it is exactly the path where the guidance needs to be most precise.