diff --git a/src/basic_memory/api/v2/routers/knowledge_router.py b/src/basic_memory/api/v2/routers/knowledge_router.py index c70820e7b..3ecf5874b 100644 --- a/src/basic_memory/api/v2/routers/knowledge_router.py +++ b/src/basic_memory/api/v2/routers/knowledge_router.py @@ -21,6 +21,7 @@ import logfire from basic_memory import db +from basic_memory.services.exceptions import AmbiguousIdentifierError from basic_memory.services.directory_deletes import DirectoryDeleteServiceError from basic_memory.services.note_content_writes import NoteContentMutationServiceError from basic_memory.ignore_utils import ( @@ -276,12 +277,20 @@ async def resolve_identifier( resolution_method = "external_id" if entity else "search" if not entity: - entity = await link_resolver.resolve_link( - data.identifier, - source_path=data.source_path, - strict=data.strict, - session=session, - ) + try: + entity = await link_resolver.resolve_link( + data.identifier, + source_path=data.source_path, + strict=data.strict, + session=session, + ) + except AmbiguousIdentifierError as exc: + # A strict resolve refused to guess between several same-title notes (#1148). + # Surface it as 409 so edit/move report the ambiguity and ask for an exact id. + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=str(exc), + ) from exc if entity: if entity.permalink == data.identifier: resolution_method = "permalink" diff --git a/src/basic_memory/indexing/relation_resolution.py b/src/basic_memory/indexing/relation_resolution.py index 3a2ee6c0f..4c0ca5996 100644 --- a/src/basic_memory/indexing/relation_resolution.py +++ b/src/basic_memory/indexing/relation_resolution.py @@ -13,6 +13,7 @@ from basic_memory import db from basic_memory.indexing.models import IndexFileJobStatus +from basic_memory.services.exceptions import AmbiguousIdentifierError from basic_memory.models import Entity from basic_memory.repository.relation_repository import ( ResolvedRelationWrite, @@ -167,13 +168,19 @@ async def resolve_relations( f"to_name={relation.to_name}" ) if relation.to_name not in resolved_targets_by_link_text: - resolved_targets_by_link_text[ - relation.to_name - ] = await self.link_resolver.resolve_link( - relation.to_name, - strict=True, - session=session, - ) + try: + resolved_targets_by_link_text[ + relation.to_name + ] = await self.link_resolver.resolve_link( + relation.to_name, + strict=True, + session=session, + ) + except AmbiguousIdentifierError: + # The target title is shared by several notes; we can't safely pick one, + # so leave this relation a forward reference (as if unresolved) instead of + # aborting the whole pass and stranding other resolvable relations (#1148). + resolved_targets_by_link_text[relation.to_name] = None resolved_entity = resolved_targets_by_link_text[relation.to_name] if resolved_entity is None or resolved_entity.id == relation.from_id: continue diff --git a/src/basic_memory/services/exceptions.py b/src/basic_memory/services/exceptions.py index 3082fd81b..c9e28cb94 100644 --- a/src/basic_memory/services/exceptions.py +++ b/src/basic_memory/services/exceptions.py @@ -10,6 +10,28 @@ class EntityNotFoundError(Exception): pass +class AmbiguousIdentifierError(Exception): + """Raised when a non-exact identifier matches multiple entities under strict resolution. + + Strict resolution backs destructive operations (edit, move). It must never silently pick + one of several same-title notes (e.g. an original plus a ``-1`` duplicate), so the caller is + told to disambiguate with an exact permalink or external_id. Non-strict resolution (wiki + links, reads) keeps its shortest-path preference and does not raise. See issue #1148. + """ + + def __init__(self, identifier: str, candidates: list[tuple[str | None, str]]) -> None: + self.identifier = identifier + self.candidates = candidates + listing = "; ".join( + f"{file_path} (permalink: {permalink})" if permalink else f"{file_path} (no permalink)" + for permalink, file_path in candidates + ) + super().__init__( + f"Ambiguous identifier '{identifier}' matches {len(candidates)} notes: {listing}. " + "Pass an exact permalink or external_id to disambiguate." + ) + + class EntityCreationError(Exception): """Raised when an entity cannot be created""" diff --git a/src/basic_memory/services/link_resolver.py b/src/basic_memory/services/link_resolver.py index b63a29c03..cd59b0b6a 100644 --- a/src/basic_memory/services/link_resolver.py +++ b/src/basic_memory/services/link_resolver.py @@ -11,6 +11,7 @@ from basic_memory.models import Entity, Project from basic_memory.repository.entity_repository import EntityRepository from basic_memory.repository.project_repository import ProjectRepository +from basic_memory.services.exceptions import AmbiguousIdentifierError from basic_memory.repository.search_repository import create_search_repository from basic_memory.schemas.search import SearchQuery, SearchItemType from basic_memory.services.search_service import SearchService @@ -79,6 +80,11 @@ class LinkResolver: 3. Try exact file path match 4. Try file path with .md extension (for folder/title patterns) 5. Fall back to search for fuzzy matching + + When ``strict`` is set (the destructive edit/move paths) and a title matches more than one + note, resolution raises ``AmbiguousIdentifierError`` instead of guessing — the caller must pass + an exact permalink or external_id (issue #1148). Non-strict resolution keeps its shortest-path + preference. """ def __init__( @@ -352,7 +358,29 @@ async def _resolve_in_project( # Multiple candidates - pick closest to source return self._find_closest_entity(candidates, source_path) - # Standard resolution (no source context): permalink first, then title + # Standard resolution (no source context): permalink first, then title. + # + # A destructive (strict) resolve must not silently guess between several same-title notes + # — e.g. an original plus a `-1` duplicate — which is how edit/move landed on the wrong + # entity (issue #1148). The trap is that the permalink step also matches the *slug* of the + # title (`build_permalink_resolution_candidates` slugifies the input), so a duplicated title + # whose original owns the title-derived permalink would still resolve silently. Pre-read the + # title matches so the permalink loop can tell a caller-supplied exact permalink (input is + # already in slug form) from the slug of a shared title. Non-strict resolution keeps the + # fast path — the title is read only if the permalink loop misses. + strict_title_matches = ( + await entity_repository.get_by_title(session, clean_text, load_relations=load_relations) + if strict + else None + ) + strict_ambiguous_title = strict_title_matches is not None and len(strict_title_matches) > 1 + # The caller supplied an exact permalink when their verbatim (project-normalized) identifier + # matches a stored permalink. That is the first, un-slugified candidate the builder emits, + # so it also accepts explicit custom permalinks (e.g. "API_V2") that are not slug-shaped — + # inferring exactness from slug shape would wrongly reject them. Only this bypasses the + # duplicate-title guard below (#1148). + exact_identifier = normalize_project_reference(clean_text).strip("/") + # 1. Try exact permalink match first (most efficient) for candidate_permalink in permalink_candidates: entity = await entity_repository.get_by_permalink( @@ -361,20 +389,30 @@ async def _resolve_in_project( load_relations=load_relations, ) if entity: + # The slugified form of a shared title must not silently win for a destructive op; + # only the caller's exact (verbatim) permalink candidate may bypass the guard. + if strict_ambiguous_title and candidate_permalink != exact_identifier: + break logger.debug(f"Found exact permalink match: {entity.permalink}") return entity - # 2. Try exact title match - found = await entity_repository.get_by_title( - session, - clean_text, - load_relations=load_relations, + # 2. Try exact title match. An exact file-path match below is more precise than a title and + # can still disambiguate, so defer any ambiguity rejection until the path lookups have run. + found = ( + strict_title_matches + if strict + else await entity_repository.get_by_title( + session, clean_text, load_relations=load_relations + ) ) + ambiguous_title_candidates: list[Entity] = [] if found: - # Return first match (shortest path) if no source context - entity = found[0] - logger.debug(f"Found title match: {entity.title}") - return entity + if strict and len(found) > 1: + ambiguous_title_candidates = list(found) + else: + entity = found[0] + logger.debug(f"Found title match: {entity.title}") + return entity # 3. Try file path found_path = await entity_repository.get_by_file_path( @@ -398,6 +436,14 @@ async def _resolve_in_project( logger.debug(f"Found entity with path (with .md): {found_path_md.file_path}") return found_path_md + # No exact permalink or file path matched. If the only thing that matched was a title + # shared by several notes under a strict resolve, refuse to guess (#1148). + if ambiguous_title_candidates: + raise AmbiguousIdentifierError( + clean_text, + [(entity.permalink, entity.file_path) for entity in ambiguous_title_candidates], + ) + # In strict mode, don't try fuzzy search - return None if no exact match found if strict: return None diff --git a/tests/api/v2/test_knowledge_router.py b/tests/api/v2/test_knowledge_router.py index b9e9bca04..1bd9dd324 100644 --- a/tests/api/v2/test_knowledge_router.py +++ b/tests/api/v2/test_knowledge_router.py @@ -135,6 +135,34 @@ async def test_resolve_identifier_not_found(client: AsyncClient, v2_project_url) assert "Entity not found" in response.json()["detail"] +@pytest.mark.asyncio +async def test_resolve_identifier_ambiguous_title_returns_409( + client: AsyncClient, v2_project_url +): + """A strict resolve of a title shared by multiple notes returns 409 (#1148). + + Covers the router's AmbiguousIdentifierError -> 409 transport contract: the resolver raising + is not enough, edit/move rely on this handler surfacing the disambiguation message. + """ + for directory in ("reports", "archive"): + create = await client.post( + f"{v2_project_url}/knowledge/entities", + json={"title": "Duplicate Report", "directory": directory, "content": "body"}, + ) + assert create.status_code == 202 + + response = await client.post( + f"{v2_project_url}/knowledge/resolve", + json={"identifier": "Duplicate Report", "strict": True}, + ) + + assert response.status_code == 409 + detail = response.json()["detail"] + assert "Ambiguous identifier 'Duplicate Report'" in detail + assert "matches 2 notes" in detail + assert "exact permalink or external_id" in detail + + @pytest.mark.asyncio async def test_resolve_identifier_no_fuzzy_match(client: AsyncClient, v2_project_url): """Test that resolve uses strict mode - no fuzzy search fallback. diff --git a/tests/indexing/test_relation_resolution.py b/tests/indexing/test_relation_resolution.py index 912503ce8..3ac9721a0 100644 --- a/tests/indexing/test_relation_resolution.py +++ b/tests/indexing/test_relation_resolution.py @@ -495,3 +495,64 @@ async def test_repository_runtime_batches_resolution_and_entity_refresh_sessions ) ] assert FakeSession.created_count == 2 + + +@pytest.mark.asyncio +async def test_resolve_relations_skips_ambiguous_target_without_aborting_pass() -> None: + """An ambiguous relation target is left unresolved; other relations still resolve (#1148). + + strict resolution now raises AmbiguousIdentifierError when a target title matches several + notes. The repair pass must treat that as an unresolved forward reference rather than let the + exception abort the whole batch and strand other resolvable relations. + """ + from basic_memory.services.exceptions import AmbiguousIdentifierError + + class AmbiguousStubLinkResolver(StubLinkResolver): + def __init__( + self, + targets: dict[str, FakeResolvedEntity], + ambiguous: set[str], + ) -> None: + super().__init__(targets) + self.ambiguous = ambiguous + + async def resolve_link( + self, + link_text: str, + *, + strict: bool, + session: AsyncSession, + ) -> FakeResolvedEntity | None: + assert isinstance(session, FakeSession) + self.calls.append((link_text, strict)) + if link_text in self.ambiguous: + raise AmbiguousIdentifierError( + link_text, [("dup-a", "a.md"), ("dup-b", "b.md")] + ) + return self.targets.get(link_text) + + repo = StubRelationRepository( + [ + [ + FakeRelation(id=1, from_id=10, to_name="Ambiguous Title"), + FakeRelation(id=2, from_id=11, to_name="Clear Target"), + ] + ] + ) + link_resolver = AmbiguousStubLinkResolver( + targets={"Clear Target": FakeResolvedEntity(id=20, title="Clear Target")}, + ambiguous={"Ambiguous Title"}, + ) + entity_indexer = StubEntityIndexer() + runtime = build_repository_runtime(repo, link_resolver, entity_indexer) + + # The pass completes (no exception propagates) ... + affected = await runtime.resolve_relations() + + # ... resolving only the unambiguous relation; the ambiguous one stays a forward reference. + assert affected == {11} + assert len(repo.write_batches) == 1 + writes = repo.write_batches[0] + assert [write.relation_id for write in writes] == [2] + assert writes[0].target_id == 20 + assert ("Ambiguous Title", True) in link_resolver.calls diff --git a/tests/services/test_link_resolver.py b/tests/services/test_link_resolver.py index 302652f40..6e6d46e6d 100644 --- a/tests/services/test_link_resolver.py +++ b/tests/services/test_link_resolver.py @@ -486,10 +486,11 @@ async def test_exact_match_types_in_strict_mode(link_resolver, test_entities, pr assert result is not None assert result.permalink == f"{project_prefix}/components/core-service" - # 2. Exact title match - result = await link_resolver.resolve_link("Core Service", strict=True) + # 2. Exact title match (unique title — an ambiguous title is covered separately by + # test_duplicate_title_raises_ambiguous_in_strict_mode). + result = await link_resolver.resolve_link("Auth Service", strict=True) assert result is not None - assert result.permalink == f"{project_prefix}/components/core-service" + assert result.permalink == f"{project_prefix}/components/auth-service" # 3. Exact file path match result = await link_resolver.resolve_link("components/Core Service.md", strict=True) @@ -530,11 +531,13 @@ async def test_fuzzy_matching_blocked_in_strict_mode(link_resolver, test_entitie async def test_link_normalization_with_strict_mode(link_resolver, test_entities, project_prefix): """Test that link normalization still works in strict mode.""" - # Test bracket removal and alias handling in strict mode + # Test bracket removal and alias handling in strict mode. Use a unique title so this + # exercises normalization, not duplicate-title resolution (ambiguity is covered by + # test_duplicate_title_raises_ambiguous_in_strict_mode). queries_and_expected = [ - ("[[Core Service]]", f"{project_prefix}/components/core-service"), - ("[[Core Service|Main]]", f"{project_prefix}/components/core-service"), - (" [[ Core Service ]] ", f"{project_prefix}/components/core-service"), + ("[[Auth Service]]", f"{project_prefix}/components/auth-service"), + ("[[Auth Service|Main]]", f"{project_prefix}/components/auth-service"), + (" [[ Auth Service ]] ", f"{project_prefix}/components/auth-service"), ] for query, expected_permalink in queries_and_expected: @@ -544,21 +547,189 @@ async def test_link_normalization_with_strict_mode(link_resolver, test_entities, @pytest.mark.asyncio -async def test_duplicate_title_handling_in_strict_mode( +async def test_duplicate_title_raises_ambiguous_in_strict_mode( link_resolver, test_entities, project_prefix ): - """Test how duplicate titles are handled in strict mode.""" + """Strict resolution refuses to guess between same-title notes (#1148). + + "Core Service" appears twice (components/core-service and components2/core-service). A + destructive (strict) resolve — edit_note / move_note — must fail loud instead of silently + picking the shortest path, which is how those tools landed on the wrong entity when an + original and a `-1` duplicate coexisted. + """ + from basic_memory.services.exceptions import AmbiguousIdentifierError + + with pytest.raises(AmbiguousIdentifierError) as exc_info: + await link_resolver.resolve_link("Core Service", strict=True) + + message = str(exc_info.value) + assert "Core Service" in message + assert f"{project_prefix}/components/core-service" in message + assert f"{project_prefix}/components2/core-service" in message + assert "exact permalink or external_id" in message + # Both candidates are exposed for programmatic handling. + assert len(exc_info.value.candidates) == 2 - # "Core Service" appears twice in test data (components/core-service and components2/core-service) - # In strict mode, if there are multiple exact title matches, it should still return the first one - # (same behavior as normal mode for exact matches) - result = await link_resolver.resolve_link("Core Service", strict=True) +@pytest.mark.asyncio +async def test_duplicate_title_non_strict_keeps_shortest_path(link_resolver, project_prefix): + """Non-strict resolution (wiki links, reads) still picks shortest path and never raises.""" + result = await link_resolver.resolve_link("Core Service", strict=False) assert result is not None - # Should return the first match (components/core-service based on test fixture order) assert result.permalink == f"{project_prefix}/components/core-service" +@pytest.mark.asyncio +async def test_unique_title_still_resolves_in_strict_mode(link_resolver, project_prefix): + """A title with a single match is unaffected — strict resolution returns it, no error.""" + result = await link_resolver.resolve_link("Auth Service", strict=True) + assert result is not None + assert result.permalink == f"{project_prefix}/components/auth-service" + + +@pytest.mark.asyncio +async def test_exact_file_path_wins_over_ambiguous_title_in_strict_mode( + entity_repository, session_maker, link_resolver +): + """An exact, unique file path disambiguates even when the title is shared (#1148, P2). + + Two notes share the title "Diagram.png"; only one has file_path equal to the identifier. + File paths are part of the resolver contract and more precise than a title, so strict + resolution must return the exact-path match rather than raising AmbiguousIdentifierError. + Permalinks here are non-derived so the permalink step does not short-circuit first. + """ + now = datetime.now(timezone.utc) + async with db.scoped_session(session_maker) as session: + await entity_repository.add( + session, + EntityModel( + title="Diagram.png", + note_type="file", + content_type="image/png", + file_path="Diagram.png", + permalink="diagram-root", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + await entity_repository.add( + session, + EntityModel( + title="Diagram.png", + note_type="file", + content_type="image/png", + file_path="archive/old-diagram.png", + permalink="diagram-archive", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + + result = await link_resolver.resolve_link("Diagram.png", strict=True) + assert result is not None + assert result.file_path == "Diagram.png" + assert result.permalink == "diagram-root" + + +@pytest.mark.asyncio +async def test_ambiguous_title_not_bypassed_via_title_derived_permalink_in_strict_mode( + entity_repository, session_maker, link_resolver +): + """A duplicated title must not resolve via its own slug under a strict op (#1148, P1 follow-up). + + The original owns the title-derived permalink ("widget"); a duplicate got "widget-1". + build_permalink_resolution_candidates slugifies "Widget" to "widget", so without the guard the + permalink step would silently return the original for edit/move. A strict resolve of the bare + (ambiguous) title must raise; an exact permalink still resolves precisely. + """ + from basic_memory.services.exceptions import AmbiguousIdentifierError + + now = datetime.now(timezone.utc) + async with db.scoped_session(session_maker) as session: + await entity_repository.add( + session, + EntityModel( + title="Widget", + note_type="note", + content_type="text/markdown", + file_path="Widget.md", + permalink="widget", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + await entity_repository.add( + session, + EntityModel( + title="Widget", + note_type="note", + content_type="text/markdown", + file_path="archive/Widget.md", + permalink="widget-1", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + + # Bare, ambiguous title raises even though a title-derived permalink ("widget") exists. + with pytest.raises(AmbiguousIdentifierError): + await link_resolver.resolve_link("Widget", strict=True) + + # An exact permalink is a precise pointer and still resolves. + result = await link_resolver.resolve_link("widget", strict=True) + assert result is not None + assert result.permalink == "widget" + + +@pytest.mark.asyncio +async def test_exact_custom_permalink_resolves_despite_ambiguous_title_in_strict_mode( + entity_repository, session_maker, link_resolver +): + """An explicit non-slug permalink is accepted verbatim even when the title is shared (#1148). + + Custom frontmatter permalinks (e.g. "API_V2") are not slug-shaped, so exactness must come from + the raw candidate that matched, not from slug shape. A caller passing that exact permalink must + resolve to its entity — otherwise the ambiguity error would name a permalink the resolver then + refuses to accept. + """ + now = datetime.now(timezone.utc) + async with db.scoped_session(session_maker) as session: + await entity_repository.add( + session, + EntityModel( + title="API_V2", + note_type="note", + content_type="text/markdown", + file_path="API_V2.md", + permalink="API_V2", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + await entity_repository.add( + session, + EntityModel( + title="API_V2", + note_type="note", + content_type="text/markdown", + file_path="archive/API_V2.md", + permalink="api-v2-1", + created_at=now, + updated_at=now, + project_id=entity_repository.project_id, + ), + ) + + result = await link_resolver.resolve_link("API_V2", strict=True) + assert result is not None + assert result.permalink == "API_V2" + + @pytest.mark.asyncio async def test_cross_project_link_resolution( session_maker, entity_repository, search_service, tmp_path, app_config