From 3f1b8db40b5de7473eadb3241e8192c44787cf3e Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 25 Aug 2026 13:10:18 -0500 Subject: [PATCH 1/2] fix(api): resolve note directories case-insensitively MCP write_note with directory "schemas" beside an existing "Schemas/" silently created a case-duplicate sibling folder on case-sensitive storage (Basic Memory Cloud); move_note's destination parent had the same failure mode. The requested directory now resolves against the project's known folders, derived from indexed entity file paths in the database (the same source list_directory uses), so local and cloud runtimes behave identically and storage is never probed for casing. Resolution rules, applied per path segment against children of the already-resolved parent: - Exact match wins (no behavior change). - Unique case-insensitive match adopts the existing folder's casing. - Zero matches create the folder as given (today's behavior). - Multiple existing case-variant folders are ambiguous; the requested casing is kept (today's exact behavior). The resolution lives in the accepted-note mutation runner shared by the create (POST), replace (PUT), and move flows in both runtimes. The move_note MCP tool's outcome backstop now accepts a case-only difference between the requested and actual landing path, since that is this resolution at work rather than a cross-boundary degradation. Fixes #1326 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01G4rbaeHJN3L7CREp5v38J9 Signed-off-by: phernandez --- CHANGELOG.md | 10 + .../indexing/accepted_note_mutation_runner.py | 96 +++++++- src/basic_memory/mcp/tools/move_note.py | 8 +- src/basic_memory/utils.py | 46 ++++ tests/api/v2/test_knowledge_router.py | 110 +++++++++ .../test_accepted_note_mutation_runner.py | 214 ++++++++++++++++++ tests/mcp/test_tool_move_note.py | 30 +++ tests/mcp/test_tool_write_note.py | 25 ++ tests/utils/test_resolve_directory_casing.py | 63 ++++++ 9 files changed, 594 insertions(+), 8 deletions(-) create mode 100644 tests/utils/test_resolve_directory_casing.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 197f1f747..fc4ba93fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ ## Unreleased +### Bug Fixes + +- `write_note` and `move_note` no longer create case-duplicate folders + (`schemas/` beside an existing `Schemas/`). When a requested directory is + not an existing folder but matches exactly one existing folder + case-insensitively, the note lands in the existing folder; unknown folders + are still created as given, and existing case-variant siblings keep today's + exact behavior. Folders are resolved from the indexed database paths, so + local and cloud runtimes behave identically. (#1326) + ## v0.23.1 (2026-08-25) Fast-follow patch to v0.23.0 focused on PostgreSQL search parity and a diff --git a/src/basic_memory/indexing/accepted_note_mutation_runner.py b/src/basic_memory/indexing/accepted_note_mutation_runner.py index 458b5c7d9..58407da82 100644 --- a/src/basic_memory/indexing/accepted_note_mutation_runner.py +++ b/src/basic_memory/indexing/accepted_note_mutation_runner.py @@ -60,6 +60,7 @@ ) from basic_memory.schemas.base import Entity as EntitySchema from basic_memory.schemas.request import EditEntityRequest +from basic_memory.utils import resolve_directory_casing type AcceptedNoteMutationChange = RuntimeAcceptedNoteChange[RuntimeNoteContentResponsePayload] type AcceptedNoteMutationUserProfileId = UUID @@ -233,6 +234,11 @@ async def get_by_file_path( load_relations: bool = False, ) -> Entity | None: ... + async def get_distinct_directories( + self, + session: AsyncSession, + ) -> list[str]: ... + class AcceptedNoteMutationNoteContentRepository(Protocol): """note_content lookup capability for accepted-note mutations.""" @@ -379,6 +385,58 @@ async def resolve_accepted_note_source_checksum( ) +async def resolve_accepted_note_directory( + session: AsyncSession, + *, + project_id: ProjectId, + directory: str, + dependencies: AcceptedNoteMutationDependencies, +) -> str: + """Resolve a requested note directory against existing folder casing (#1326). + + Trigger: the requested directory is not an existing folder but matches + exactly one existing folder case-insensitively. + Why: LLM callers guess plausible casing ("schemas" beside an existing + "Schemas/"); on case-sensitive storage (cloud object storage) the guess + silently creates a case-duplicate sibling folder. Folders are derived + from indexed entity file paths in the DB, never by probing storage, so + local and cloud runtimes resolve identically. + Outcome: a unique case-insensitive match adopts the existing folder's + casing; exact matches, unknown folders, and ambiguous case-variant + siblings keep the requested casing unchanged. + """ + if not directory: + return directory + entity_repository = dependencies.lookup_repositories.entity_repository(project_id) + existing_directories = await entity_repository.get_distinct_directories(session) + return resolve_directory_casing(directory, existing_directories) + + +async def resolve_accepted_note_schema_directory( + session: AsyncSession, + *, + project_id: ProjectId, + data: EntitySchema, + dependencies: AcceptedNoteMutationDependencies, +) -> EntitySchema: + """Return the write schema with its directory resolved to existing casing. + + The schema's ``file_path`` is computed from ``directory``, so adjusting the + directory redirects the conflict lookup, preparation, and permalink + resolution downstream. The caller's schema is never mutated: a resolved + directory yields a copy so route-owned request data stays as received. + """ + resolved_directory = await resolve_accepted_note_directory( + session, + project_id=project_id, + directory=data.directory, + dependencies=dependencies, + ) + if resolved_directory == data.directory: + return data + return data.model_copy(update={"directory": resolved_directory}) + + async def run_accepted_note_create( session: AsyncSession, *, @@ -501,10 +559,16 @@ async def _run_accepted_note_create( dependencies=dependencies, ) + data = await resolve_accepted_note_schema_directory( + session, + project_id=project.id, + data=request.data, + dependencies=dependencies, + ) entity_repository = dependencies.lookup_repositories.entity_repository(project.id) conflicting_entity = await entity_repository.get_by_file_path( session, - request.data.file_path, + data.file_path, load_relations=False, ) reject_accepted_note_file_path_conflict( @@ -515,7 +579,7 @@ async def _run_accepted_note_create( preparer = dependencies.preparer_factory.create_note_preparer(project) prepared_write = await prepare_create_or_reject( preparer, - request.data, + data, check_storage_exists=dependencies.verify_storage_absent_on_create, session=session, ) @@ -579,10 +643,16 @@ async def _run_accepted_note_update( existing_file_path = entity.file_path if entity is not None else None vacated_source: tuple[RuntimeFilePath, RuntimeFileChecksum | None] | None = None + data = await resolve_accepted_note_schema_directory( + session, + project_id=project.id, + data=request.data, + dependencies=dependencies, + ) await reject_conflicting_accepted_note_file_path( session, project_id=project.id, - file_path=request.data.file_path, + file_path=data.file_path, allowed_entity_external_id=request.entity_external_id, dependencies=dependencies, ) @@ -600,7 +670,7 @@ async def _run_accepted_note_update( reject_stale_base_checksum(current_db_checksum=None) prepared_write = await prepare_create_or_reject( preparer, - request.data, + data, check_storage_exists=dependencies.verify_storage_absent_on_create, session=session, ) @@ -630,7 +700,7 @@ async def _run_accepted_note_update( try: await preparer.verify_move_destination_absent( source_file_path=entity.file_path, - destination_file_path=request.data.file_path, + destination_file_path=data.file_path, ) except EntityAlreadyExistsError as error: reject_accepted_note_mutation(AcceptedNoteMutationRejectKind.conflict, str(error)) @@ -644,7 +714,7 @@ async def _run_accepted_note_update( # A PUT replacement may also rename the note. Capture the exact source bytes before # persistence mutates the entity and note_content to the destination version so delayed # cleanup cannot let a later project index recreate the old path as a ghost. - if request.data.file_path != entity.file_path: + if data.file_path != entity.file_path: vacated_source = ( entity.file_path, await resolve_accepted_note_source_checksum( @@ -693,7 +763,7 @@ async def _run_accepted_note_update( preparer, session, entity=entity, - data=request.data, + data=data, current_note_content=current_note_content, user_profile_value=user_profile_value, ) @@ -825,6 +895,18 @@ async def _run_accepted_note_move( dependencies=dependencies, ) existing_file_path = entity.file_path + # The destination filename keeps its requested casing; only the parent + # directory resolves against existing folders (issue #1326). The path is + # posix-normalized above, so rpartition splits directory from filename. + destination_directory, _, destination_filename = accepted_file_path.rpartition("/") + resolved_directory = await resolve_accepted_note_directory( + session, + project_id=project.id, + directory=destination_directory, + dependencies=dependencies, + ) + if resolved_directory != destination_directory: + accepted_file_path = f"{resolved_directory}/{destination_filename}" # Same-path moves fail fast everywhere by decision (2026-07-14): cloud's # pre-unification route returned a 200 no-op, local rejected — the unified # runner keeps the rejection so a mistaken identity move surfaces instead diff --git a/src/basic_memory/mcp/tools/move_note.py b/src/basic_memory/mcp/tools/move_note.py index 83e57a348..118c92b1a 100644 --- a/src/basic_memory/mcp/tools/move_note.py +++ b/src/basic_memory/mcp/tools/move_note.py @@ -947,9 +947,15 @@ async def _ensure_resolved_entity_id() -> str: # the up-front detection: any divergence the caller did not ask for # surfaces as a failure rather than a fake success. # Outcome: report failure with the actual landing path instead of "✅". + # A case-only difference is the server resolving the destination folder to + # an existing folder's casing (#1326), not a boundary degradation — the + # success message below reports the actual landing path either way. normalized_requested = PureWindowsPath(destination_path).as_posix().strip("/") normalized_actual = PureWindowsPath(result.file_path).as_posix().strip("/") - if normalized_actual != normalized_requested: + if ( + normalized_actual != normalized_requested + and normalized_actual.lower() != normalized_requested.lower() + ): logger.warning( f"Move outcome diverged from intent: requested={destination_path} " f"actual={result.file_path}" diff --git a/src/basic_memory/utils.py b/src/basic_memory/utils.py index 6f5df5c24..1e9606675 100644 --- a/src/basic_memory/utils.py +++ b/src/basic_memory/utils.py @@ -776,6 +776,52 @@ def detect_potential_file_conflicts(file_path: str, existing_paths: List[str]) - return conflicts +def resolve_directory_casing(directory: str, existing_directories: List[str]) -> str: + """Resolve a requested directory to the casing of existing folders (#1326). + + LLM callers guess plausible-but-wrong casing ("schemas" beside an existing + "Schemas/"), and case-sensitive storage backends then grow case-duplicate + sibling folders. Each path segment resolves against the folders that exist + under the already-resolved parent: + + - Exact match wins: the requested segment casing is kept unchanged. + - Unique case-insensitive match: the existing folder's casing is adopted. + - No match: the segment is kept as given (a new folder). + - Multiple case-variant folders exist: ambiguous, the segment is kept as + given (today's exact behavior). + + Args: + directory: Requested directory path relative to the project root + ("" means the root itself). + existing_directories: Known project folders derived from indexed entity + file paths (see EntityRepository.get_distinct_directories). + + Returns: + The directory path with each segment resolved to existing casing. + """ + if not directory: + return directory + + existing = set(existing_directories) + resolved_prefix = "" + for segment in directory.split("/"): + candidate = f"{resolved_prefix}/{segment}" if resolved_prefix else segment + if candidate in existing: + resolved_prefix = candidate + continue + # Candidates are constrained to children of the already-resolved parent: + # once a parent segment stays ambiguous (kept as requested), a deeper + # existing folder under a *different* parent casing must not be spliced in. + matches = [ + existing_directory + for existing_directory in existing + if existing_directory.lower() == candidate.lower() + and existing_directory.rpartition("/")[0] == resolved_prefix + ] + resolved_prefix = matches[0] if len(matches) == 1 else candidate + return resolved_prefix + + def valid_project_path_value(path: str): """Ensure project path is valid.""" # Allow empty strings as they resolve to the project root diff --git a/tests/api/v2/test_knowledge_router.py b/tests/api/v2/test_knowledge_router.py index f76e20b14..a46ecb8b3 100644 --- a/tests/api/v2/test_knowledge_router.py +++ b/tests/api/v2/test_knowledge_router.py @@ -394,6 +394,88 @@ async def test_create_entity(client: AsyncClient, file_service, v2_project_url): assert data["content"] in file_content +@pytest.mark.asyncio +async def test_create_entity_resolves_directory_casing( + client: AsyncClient, file_service, v2_project_url +): + """A unique case-insensitive folder match adopts the existing casing (#1326).""" + seed = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Call", "directory": "Schemas", "content": "Call schema"}, + ) + assert seed.status_code == 202 + + response = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Research", "directory": "schemas", "content": "Research schema"}, + ) + + assert response.status_code == 202 + entity = EntityResponseV2.model_validate(response.json()) + assert entity.file_path == "Schemas/Research.md" + + file_content, _ = await file_service.read_file("Schemas/Research.md") + assert "Research schema" in file_content + + +@pytest.mark.asyncio +async def test_create_entity_resolves_nested_directory_casing(client: AsyncClient, v2_project_url): + """Each nested path segment resolves against existing folders (#1326).""" + seed = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Seed", "directory": "Schemas/Drafts", "content": "Seed"}, + ) + assert seed.status_code == 202 + + response = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Nested", "directory": "schemas/drafts", "content": "Nested"}, + ) + + assert response.status_code == 202 + entity = EntityResponseV2.model_validate(response.json()) + assert entity.file_path == "Schemas/Drafts/Nested.md" + + +@pytest.mark.asyncio +async def test_create_entity_keeps_ambiguous_directory_casing( + client: AsyncClient, + v2_project_url, + test_project: Project, + entity_repository, + session_maker, +): + """Existing case-variant sibling folders keep today's exact behavior (#1326).""" + now = datetime.now(timezone.utc) + async with db.scoped_session(session_maker) as session: + for file_path, permalink in ( + ("Schemas/One.md", "schemas/one"), + ("SCHEMAS/Two.md", "schemas/two"), + ): + await entity_repository.add( + session, + EntityModel( + title=Path(file_path).stem, + note_type="note", + content_type="text/markdown", + file_path=file_path, + permalink=permalink, + created_at=now, + updated_at=now, + project_id=test_project.id, + ), + ) + + response = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Three", "directory": "schemas", "content": "Ambiguous"}, + ) + + assert response.status_code == 202 + entity = EntityResponseV2.model_validate(response.json()) + assert entity.file_path == "schemas/Three.md" + + @pytest.mark.asyncio async def test_create_entity_conflict_returns_409(client: AsyncClient, v2_project_url): """Test creating a duplicate entity returns 409 Conflict.""" @@ -1318,6 +1400,34 @@ async def test_move_entity( assert note_content.file_write_status == "synced" +@pytest.mark.asyncio +async def test_move_entity_resolves_destination_directory_casing( + client: AsyncClient, v2_project_url +): + """A move destination parent adopts the unique existing folder casing (#1326).""" + seed = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Seed", "directory": "Schemas", "content": "Seed"}, + ) + assert seed.status_code == 202 + + create = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "MoveMe", "directory": "test", "content": "Content to move"}, + ) + assert create.status_code == 202 + source_external_id = EntityResponseV2.model_validate(create.json()).external_id + + response = await client.put( + f"{v2_project_url}/knowledge/entities/{source_external_id}/move", + json={"destination_path": "schemas/MoveMe.md"}, + ) + + assert response.status_code == 202 + moved = EntityResponseV2.model_validate(response.json()) + assert moved.file_path == "Schemas/MoveMe.md" + + @pytest.mark.asyncio async def test_move_entity_rejects_existing_unindexed_destination( client: AsyncClient, v2_project_url, project_config diff --git a/tests/indexing/test_accepted_note_mutation_runner.py b/tests/indexing/test_accepted_note_mutation_runner.py index 3966d99ab..2f3f36542 100644 --- a/tests/indexing/test_accepted_note_mutation_runner.py +++ b/tests/indexing/test_accepted_note_mutation_runner.py @@ -346,11 +346,14 @@ def __init__( *, by_external_id: Entity | None = None, by_file_path: Entity | None = None, + distinct_directories: list[str] | None = None, ) -> None: self.by_external_id = by_external_id self.by_file_path = by_file_path + self.distinct_directories = distinct_directories or [] self.external_id_calls: list[tuple[AsyncSession, str, bool]] = [] self.file_path_calls: list[tuple[AsyncSession, str, bool]] = [] + self.distinct_directory_calls: list[AsyncSession] = [] async def get_by_external_id( self, @@ -372,6 +375,13 @@ async def get_by_file_path( self.file_path_calls.append((session, file_path, load_relations)) return self.by_file_path + async def get_distinct_directories( + self, + session: AsyncSession, + ) -> list[str]: + self.distinct_directory_calls.append(session) + return self.distinct_directories + class _NoteContentLookupRepository: def __init__(self, note_content: NoteContent | None = None) -> None: @@ -1687,6 +1697,210 @@ async def test_run_accepted_note_move_rejects_same_file_path() -> None: assert exc_info.value.rejection.detail == "Source and destination paths are the same." +@pytest.mark.asyncio +async def test_run_accepted_note_create_resolves_directory_casing() -> None: + """A unique case-insensitive folder match redirects the create (#1326).""" + session = cast(AsyncSession, object()) + schema = _schema() + project = _project() + entity = _entity() + entity_lookup_repository = _EntityLookupRepository(distinct_directories=["Notes", "specs"]) + preparer = _CreatePreparer(_prepared()) + + await run_accepted_note_create( + session, + request=AcceptedNoteCreateMutation( + project_external_id="project-123", + data=schema, + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(_note_content(entity)), + search_repository=_SearchRepository(), + ), + ) + + assert entity_lookup_repository.distinct_directory_calls == [session] + # Conflict lookup, filename conflict detection, and preparation all see the + # resolved existing casing. + assert entity_lookup_repository.file_path_calls == [(session, "Notes/Accepted.md", False)] + assert preparer.conflict_calls == [("Notes/Accepted.md", False, session)] + prepared_schema = preparer.calls[0][0] + assert prepared_schema.directory == "Notes" + assert prepared_schema.file_path == "Notes/Accepted.md" + # The route-owned request schema stays as received. + assert schema.directory == "notes" + + +@pytest.mark.asyncio +async def test_run_accepted_note_create_keeps_ambiguous_directory_casing() -> None: + """Multiple existing case-variant folders keep today's exact behavior.""" + session = cast(AsyncSession, object()) + schema = _schema() + project = _project() + entity = _entity() + entity_lookup_repository = _EntityLookupRepository(distinct_directories=["Notes", "NOTES"]) + preparer = _CreatePreparer(_prepared()) + + await run_accepted_note_create( + session, + request=AcceptedNoteCreateMutation( + project_external_id="project-123", + data=schema, + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(_note_content(entity)), + search_repository=_SearchRepository(), + ), + ) + + assert entity_lookup_repository.file_path_calls == [(session, "notes/Accepted.md", False)] + # The unchanged schema is passed through without copying. + assert preparer.calls == [(schema, False, session)] + + +@pytest.mark.asyncio +async def test_run_accepted_note_update_resolves_directory_casing() -> None: + """A PUT with a case-variant directory replaces in place instead of renaming.""" + session = _MutationSession() + schema = _schema() + project = _project() + entity = _entity(file_path="Notes/Accepted.md") + note_content = _note_content(entity) + entity_lookup_repository = _EntityLookupRepository( + by_external_id=entity, + distinct_directories=["Notes"], + ) + preparer = _CreatePreparer(_prepared_replacement()) + preparer_factory = _PreparerFactory(preparer) + + await run_accepted_note_update( + cast(AsyncSession, session), + request=AcceptedNoteUpdateMutation( + project_external_id="project-123", + entity_external_id="note-123", + data=schema, + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=preparer_factory, + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + ), + ) + + replaced_schema = preparer.replace_calls[0][1] + assert replaced_schema.directory == "Notes" + assert replaced_schema.file_path == "Notes/Accepted.md" + # No rename happened, so no source path was vacated for cleanup. + assert preparer_factory.checksum_calls == [] + + +@pytest.mark.asyncio +async def test_run_accepted_note_move_resolves_destination_directory_casing() -> None: + """A move destination parent adopts the unique existing folder casing.""" + session = _MutationSession() + project = _project() + entity = _entity(file_path="notes/accepted.md") + note_content = _note_content(entity) + prepared_move = PreparedEntityMove( + file_path=Path("Archive/accepted.md"), + markdown_content="# Moved\n", + search_content="Moved", + permalink="archive/accepted", + ) + entity_lookup_repository = _EntityLookupRepository( + by_external_id=entity, + distinct_directories=["Archive", "notes"], + ) + preparer = _CreatePreparer(_prepared(), prepared_move=prepared_move) + + result = await run_accepted_note_move( + cast(AsyncSession, session), + request=AcceptedNoteMoveMutation( + project_external_id="project-123", + entity_external_id="note-123", + destination_path="archive/accepted.md", + actor=AcceptedNoteMutationActor(user_profile_id=None), + source="mcp", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + ), + ) + + assert preparer.move_calls == [ + (entity, "# Old\n", "Archive/accepted.md", False, cast(AsyncSession, session)) + ] + assert entity.file_path == "Archive/accepted.md" + change = result.change + assert change.materialization is not None + assert change.materialization.previous_file_path == "notes/accepted.md" + + +@pytest.mark.asyncio +async def test_run_accepted_note_move_rejects_case_variant_of_current_path() -> None: + """A destination resolving onto the note's own path is a same-path move.""" + session = _MutationSession() + project = _project() + entity = _entity(file_path="Notes/accepted.md") + note_content = _note_content(entity) + entity_lookup_repository = _EntityLookupRepository( + by_external_id=entity, + distinct_directories=["Notes"], + ) + preparer = _CreatePreparer(_prepared()) + + with pytest.raises(AcceptedNoteMutationRejected) as exc_info: + await run_accepted_note_move( + cast(AsyncSession, session), + request=AcceptedNoteMoveMutation( + project_external_id="project-123", + entity_external_id="note-123", + destination_path="notes/accepted.md", + actor=AcceptedNoteMutationActor(user_profile_id=None), + source="mcp", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + ), + ) + + assert exc_info.value.rejection.kind is AcceptedNoteMutationRejectKind.bad_request + assert exc_info.value.rejection.detail == "Source and destination paths are the same." + assert preparer.move_calls == [] + + @pytest.mark.asyncio async def test_run_accepted_note_delete_removes_entity_and_returns_cleanup() -> None: session = _MutationSession() diff --git a/tests/mcp/test_tool_move_note.py b/tests/mcp/test_tool_move_note.py index 53bb75a8d..46869302c 100644 --- a/tests/mcp/test_tool_move_note.py +++ b/tests/mcp/test_tool_move_note.py @@ -141,6 +141,36 @@ async def test_move_note_success(app, client, test_project): assert f"permalink: {test_project.name}/target/moved-note" in content +@pytest.mark.asyncio +async def test_move_note_resolves_destination_directory_casing(app, client, test_project): + """A case-variant destination folder resolves to the existing casing (#1326).""" + await write_note( + project=test_project.name, + title="Seed", + directory="Schemas", + content="# Seed\nExisting schema folder", + ) + await write_note( + project=test_project.name, + title="Research", + directory="test", + content="# Research\nContent to move", + ) + + result = await move_note( + project=test_project.name, + identifier="test/research", + destination_path="schemas/Research.md", + ) + + assert isinstance(result, str) + assert "✅ Note moved successfully" in result + assert "Schemas/Research.md" in result + + content = await read_note("schemas/research", project=test_project.name) + assert "Content to move" in content + + @pytest.mark.asyncio async def test_move_note_with_folder_creation(client, test_project): """Test moving note creates necessary folders.""" diff --git a/tests/mcp/test_tool_write_note.py b/tests/mcp/test_tool_write_note.py index bc5c25943..6e1ba3228 100644 --- a/tests/mcp/test_tool_write_note.py +++ b/tests/mcp/test_tool_write_note.py @@ -114,6 +114,31 @@ async def test_write_note_workspace_invalid_raises_before_routing(app, test_proj ) +@pytest.mark.asyncio +async def test_write_note_resolves_directory_casing(app, test_project): + """A case-variant directory resolves to the existing folder's casing (#1326). + + write_note with directory "schemas" beside an existing "Schemas/" must land + in "Schemas/" instead of creating a case-duplicate sibling folder. + """ + await write_note( + project=test_project.name, + title="Call", + directory="Schemas", + content="# Call\nCall schema", + ) + + result = await write_note( + project=test_project.name, + title="Research", + directory="schemas", + content="# Research\nResearch schema", + ) + + assert "# Created note" in result + assert "file_path: Schemas/Research.md" in result + + @pytest.mark.asyncio async def test_write_note(app, test_project): """Test creating a new note. diff --git a/tests/utils/test_resolve_directory_casing.py b/tests/utils/test_resolve_directory_casing.py new file mode 100644 index 000000000..a0eb9155d --- /dev/null +++ b/tests/utils/test_resolve_directory_casing.py @@ -0,0 +1,63 @@ +"""Tests for case-insensitive directory resolution (issue #1326).""" + +from basic_memory.utils import resolve_directory_casing + + +def test_exact_match_keeps_requested_casing(): + """An exactly matching folder is used as given, even beside case-variants.""" + existing = ["Schemas", "schemas", "notes"] + assert resolve_directory_casing("schemas", existing) == "schemas" + assert resolve_directory_casing("Schemas", existing) == "Schemas" + + +def test_unique_case_insensitive_match_adopts_existing_casing(): + existing = ["Schemas", "notes"] + assert resolve_directory_casing("schemas", existing) == "Schemas" + assert resolve_directory_casing("SCHEMAS", existing) == "Schemas" + assert resolve_directory_casing("NOTES", existing) == "notes" + + +def test_zero_matches_creates_as_given(): + existing = ["Schemas", "notes"] + assert resolve_directory_casing("research", existing) == "research" + assert resolve_directory_casing("Research/Drafts", existing) == "Research/Drafts" + + +def test_multiple_case_variants_keep_requested_casing(): + """Ambiguous case-variant siblings preserve today's exact behavior.""" + existing = ["Schemas", "SCHEMAS", "notes"] + assert resolve_directory_casing("schemas", existing) == "schemas" + + +def test_nested_path_resolves_each_segment(): + existing = ["Schemas", "Schemas/Drafts"] + assert resolve_directory_casing("schemas/drafts", existing) == "Schemas/Drafts" + + +def test_nested_new_subfolder_under_resolved_parent(): + existing = ["Schemas"] + assert resolve_directory_casing("schemas/drafts", existing) == "Schemas/drafts" + + +def test_ambiguous_parent_does_not_splice_other_parent_children(): + """A child under a differently-cased parent must not be adopted once the + parent segment stayed ambiguous (kept as requested).""" + existing = ["Schemas", "SCHEMAS", "SCHEMAS/Drafts"] + assert resolve_directory_casing("schemas/drafts", existing) == "schemas/drafts" + + +def test_child_resolution_requires_resolved_parent(): + """A case-insensitive whole-path match under a different parent casing is + ignored: each segment resolves only against children of the resolved parent.""" + existing = ["SCHEMAS", "SCHEMAS/Drafts", "Schemas"] + # "Schemas" matches exactly, so children resolve under "Schemas" — which has + # none — leaving the child segment as given. + assert resolve_directory_casing("Schemas/drafts", existing) == "Schemas/drafts" + + +def test_root_directory_is_unchanged(): + assert resolve_directory_casing("", ["Schemas"]) == "" + + +def test_no_existing_directories_keeps_requested(): + assert resolve_directory_casing("schemas", []) == "schemas" From d71ce6ddeec8a320cf0c842d5f4fa5be0d4ce2b6 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 25 Aug 2026 13:40:52 -0500 Subject: [PATCH 2/2] fix(api): tighten directory-casing cost and move backstop Address two Codex review findings on PR #1329: - Skip directory-casing resolution on PUT updates whose requested directory exactly matches the addressed entity's current directory. Content-only saves (e.g. repeated collaboration-relay writes) are the hot path and must not pay the O(project entities) distinct file_path scan; an exact match would resolve to itself anyway because exact match always wins. Case-variant and relocating PUTs still resolve. - The move_note outcome backstop now forgives a case-only difference only in the parent directories, comparing the basename exactly. The server-side resolution preserves the requested filename verbatim, so a basename divergence (even case-only) still reports the honest failure instead of a fake success. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01G4rbaeHJN3L7CREp5v38J9 Signed-off-by: phernandez --- .../indexing/accepted_note_mutation_runner.py | 22 ++++++--- src/basic_memory/mcp/tools/move_note.py | 19 +++++--- .../test_accepted_note_mutation_runner.py | 46 +++++++++++++++++++ tests/mcp/test_tool_move_note.py | 39 ++++++++++++++++ 4 files changed, 113 insertions(+), 13 deletions(-) diff --git a/src/basic_memory/indexing/accepted_note_mutation_runner.py b/src/basic_memory/indexing/accepted_note_mutation_runner.py index 58407da82..ffa3b9b25 100644 --- a/src/basic_memory/indexing/accepted_note_mutation_runner.py +++ b/src/basic_memory/indexing/accepted_note_mutation_runner.py @@ -643,12 +643,22 @@ async def _run_accepted_note_update( existing_file_path = entity.file_path if entity is not None else None vacated_source: tuple[RuntimeFilePath, RuntimeFileChecksum | None] | None = None - data = await resolve_accepted_note_schema_directory( - session, - project_id=project.id, - data=request.data, - dependencies=dependencies, - ) + # Trigger: the addressed entity already lives in the exact requested directory. + # Why: casing resolution scans every distinct entity file_path in the project, + # and content-only PUTs (e.g. repeated collaboration-relay saves of the + # same note) are the hot path where the directory cannot change; an exact + # match would resolve to itself anyway because exact match always wins. + # Outcome: only case-variant or relocating PUTs pay for the directory scan. + current_directory = existing_file_path.rpartition("/")[0] if existing_file_path else None + if current_directory == request.data.directory: + data = request.data + else: + data = await resolve_accepted_note_schema_directory( + session, + project_id=project.id, + data=request.data, + dependencies=dependencies, + ) await reject_conflicting_accepted_note_file_path( session, project_id=project.id, diff --git a/src/basic_memory/mcp/tools/move_note.py b/src/basic_memory/mcp/tools/move_note.py index 118c92b1a..b9f0659f4 100644 --- a/src/basic_memory/mcp/tools/move_note.py +++ b/src/basic_memory/mcp/tools/move_note.py @@ -947,15 +947,20 @@ async def _ensure_resolved_entity_id() -> str: # the up-front detection: any divergence the caller did not ask for # surfaces as a failure rather than a fake success. # Outcome: report failure with the actual landing path instead of "✅". - # A case-only difference is the server resolving the destination folder to - # an existing folder's casing (#1326), not a boundary degradation — the - # success message below reports the actual landing path either way. + # A case-only difference in the PARENT directories is the server resolving + # the destination folder to an existing folder's casing (#1326), not a + # boundary degradation — the success message below reports the actual + # landing path either way. The server always preserves the requested + # filename verbatim, so a basename divergence (even case-only) still + # reports the honest failure. normalized_requested = PureWindowsPath(destination_path).as_posix().strip("/") normalized_actual = PureWindowsPath(result.file_path).as_posix().strip("/") - if ( - normalized_actual != normalized_requested - and normalized_actual.lower() != normalized_requested.lower() - ): + requested_parent, _, requested_name = normalized_requested.rpartition("/") + actual_parent, _, actual_name = normalized_actual.rpartition("/") + diverged = normalized_actual != normalized_requested and not ( + actual_name == requested_name and actual_parent.lower() == requested_parent.lower() + ) + if diverged: logger.warning( f"Move outcome diverged from intent: requested={destination_path} " f"actual={result.file_path}" diff --git a/tests/indexing/test_accepted_note_mutation_runner.py b/tests/indexing/test_accepted_note_mutation_runner.py index 2f3f36542..5460e2d3b 100644 --- a/tests/indexing/test_accepted_note_mutation_runner.py +++ b/tests/indexing/test_accepted_note_mutation_runner.py @@ -1812,6 +1812,52 @@ async def test_run_accepted_note_update_resolves_directory_casing() -> None: assert replaced_schema.file_path == "Notes/Accepted.md" # No rename happened, so no source path was vacated for cleanup. assert preparer_factory.checksum_calls == [] + # The case-variant request paid exactly one directory scan. + assert entity_lookup_repository.distinct_directory_calls == [cast(AsyncSession, session)] + + +@pytest.mark.asyncio +async def test_run_accepted_note_update_content_only_skips_directory_scan() -> None: + """A PUT into the note's own exact directory never scans project folders. + + Regression for the PR #1329 review: content-only saves (e.g. repeated + collaboration-relay writes) are the hot path and must not pay the + O(project entities) distinct file_path scan that casing resolution costs. + """ + session = _MutationSession() + schema = _schema() + project = _project() + entity = _entity(file_path="notes/accepted.md") + note_content = _note_content(entity) + entity_lookup_repository = _EntityLookupRepository( + by_external_id=entity, + distinct_directories=["notes"], + ) + preparer = _CreatePreparer(_prepared_replacement()) + + await run_accepted_note_update( + cast(AsyncSession, session), + request=AcceptedNoteUpdateMutation( + project_external_id="project-123", + entity_external_id="note-123", + data=schema, + actor=AcceptedNoteMutationActor(user_profile_id=_ACTOR_ID), + source="api", + ), + dependencies=_dependencies( + project_repository=_ProjectRepository(project), + entity_lookup_repository=entity_lookup_repository, + note_content_lookup_repository=_NoteContentLookupRepository(note_content), + preparer_factory=_PreparerFactory(preparer), + pending_entity_repository=_PendingEntityRepository(entity), + note_content_accept_repository=_NoteContentAcceptRepository(note_content), + search_repository=_SearchRepository(), + ), + ) + + assert entity_lookup_repository.distinct_directory_calls == [] + # The unchanged request schema is passed through without copying. + assert preparer.replace_calls[0][1] is schema @pytest.mark.asyncio diff --git a/tests/mcp/test_tool_move_note.py b/tests/mcp/test_tool_move_note.py index 46869302c..f01a9d2eb 100644 --- a/tests/mcp/test_tool_move_note.py +++ b/tests/mcp/test_tool_move_note.py @@ -1299,6 +1299,45 @@ async def diverging_move_entity(self, entity_id, destination_path): assert "target/outcome-mismatch-note.md" in result assert "somewhere/else/diverged.md" in result + @pytest.mark.asyncio + async def test_move_note_basename_case_divergence_reports_failure( + self, app, client, test_project, monkeypatch + ): + """A case-only BASENAME divergence is still an honest failure. + + Directory casing resolution (#1326) only rewrites parent directories and + always preserves the requested filename verbatim, so a result whose + basename differs even by case did not come from that resolution. + """ + await write_note( + project=test_project.name, + title="Basename Case Note", + directory="source", + content="# Basename Case Note\nContent.", + ) + + from basic_memory.mcp.clients import KnowledgeClient + + real_move_entity = KnowledgeClient.move_entity + + async def diverging_move_entity(self, entity_id, destination_path): + result = await real_move_entity(self, entity_id, destination_path) + return result.model_copy(update={"file_path": "target/casenamenote.md"}) + + monkeypatch.setattr(KnowledgeClient, "move_entity", diverging_move_entity) + + result = await move_note( + project=test_project.name, + identifier="source/basename-case-note", + destination_path="target/CaseNameNote.md", + ) + + assert isinstance(result, str) + assert "✅ Note moved successfully" not in result + assert "Unexpected Result Location" in result + assert "target/CaseNameNote.md" in result + assert "target/casenamenote.md" in result + @pytest.mark.asyncio async def test_move_note_outcome_mismatch_json(self, app, client, test_project, monkeypatch): """JSON output for an outcome mismatch reports moved=False with the diagnostic."""