diff --git a/CHANGELOG.md b/CHANGELOG.md index 73e3c610e..8d289c36b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,17 @@ ### Bug Fixes +- **#1514**: Markdown path links resolve against the note's own path at resolution + time, the way wikilinks do, instead of at parse time from the file's location + on disk. The parser had derived the note's project path with `relative_to` on the + filesystem path, which raised for content parsed from anywhere outside the project + root (a hosted note read from object storage, for one) and gave a wrong base for + any other temporary location. The graph now stores the path as authored + (`../guides/Guide.md`, `./same.md`, `/root.md`); both resolvers turn it into a + project path from the source note, and background resolution keys path targets + by their source note. Wikilinks spelled `[[../x.md]]` or `[[./x.md]]` resolve + by the same rule. + - **#1558**: `search_notes(search_all_projects=True)` and `projects=[...]` rank merged full-text hits by score strength instead of raw value. SQLite bm25 scores are negative with lower meaning better, so sorting raw values put the weakest hit first diff --git a/docs/MARKDOWN_RELATIONS.md b/docs/MARKDOWN_RELATIONS.md index a558fc3a4..79fcde8d8 100644 --- a/docs/MARKDOWN_RELATIONS.md +++ b/docs/MARKDOWN_RELATIONS.md @@ -14,8 +14,16 @@ Markdown links work too. These are exact file paths. Basic Memory does not guess a title, add `.md`, apply filename aliases, or search another project when the target is missing. The graph -stores a normalized project-root target such as `/guides/Getting Started.md`; -missing targets remain unresolved and can resolve when indexed later. +stores the path as the author wrote it, relative to the note: `../guides/Getting +Started.md`, `./same.md` (a bare `same.md` is stored with the `./` mark), or a +rooted `/guides/Getting Started.md`. Resolution turns it into a project path +against the note's own location, so a note parsed from remote storage or moved +later resolves the same way; missing targets remain unresolved and can resolve +when indexed later. + +Wikilinks spelled as explicit paths follow the same rule: `[[../guides/Getting +Started.md]]` and `[[./same.md]]` resolve relative to the note and only to that +exact file. Other wikilinks keep their title, permalink, and alias resolution. External URLs, `mailto:` and `file:` links, fragment-only links, paths that escape the project, images, and links inside code do not create relations. Ordinary diff --git a/src/basic_memory/indexing/models.py b/src/basic_memory/indexing/models.py index 477cdb5d6..cabfae261 100644 --- a/src/basic_memory/indexing/models.py +++ b/src/basic_memory/indexing/models.py @@ -106,6 +106,19 @@ class IndexFrontmatterWriteResult: content: str +@dataclass(frozen=True, slots=True) +class RelationTargetRequest: + """One relation target to resolve, and the note a path target is relative to. + + Identity targets (titles, permalinks, external ids) mean the same thing from + every note, so they carry no source and one lookup serves them all. A path + target means one file per source note, so its source path is part of the key. + """ + + link_text: str + source_path: str | None = None + + @dataclass(frozen=True, slots=True) class IndexedRelation: """One parsed outgoing relation waiting for generation-owned publication.""" diff --git a/src/basic_memory/indexing/relation_resolution.py b/src/basic_memory/indexing/relation_resolution.py index d508cd410..5fe5adacd 100644 --- a/src/basic_memory/indexing/relation_resolution.py +++ b/src/basic_memory/indexing/relation_resolution.py @@ -13,7 +13,8 @@ from basic_memory import db from basic_memory.indexing.accepted_note_search import accepted_search_content_from_markdown -from basic_memory.indexing.models import IndexFileJobStatus +from basic_memory.indexing.models import IndexFileJobStatus, RelationTargetRequest +from basic_memory.markdown.path_links import is_path_target from basic_memory.models import Entity from basic_memory.repository.relation_repository import ( PendingRelationSearchRefresh, @@ -194,10 +195,10 @@ class RelationTargetBatchResolver(Protocol): async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> Mapping[str, ResolvedRelationTarget | None]: + ) -> Mapping[RelationTargetRequest, ResolvedRelationTarget | None]: """Resolve strict link targets without per-target database round-trips.""" @@ -257,6 +258,16 @@ async def count_unresolved_relations(self) -> int: async with db.scoped_session(self.session_maker) as session: return len(await self.relation_repository.find_unresolved_relations(session)) + async def _source_paths( + self, + session: AsyncSession, + relations: Sequence[UnresolvedRelation], + ) -> dict[EntityId, str]: + """Return the project path of every note that carries one of ``relations``.""" + source_ids = sorted({relation.from_id for relation in relations}) + sources = await self.entity_repository.find_by_ids(session, source_ids) + return {source.id: source.file_path for source in sources} + async def resolve_relations( self, entity_id: EntityId | None = None, @@ -280,15 +291,25 @@ async def resolve_relations( count=len(unresolved_relations), ) - target_names = list( - dict.fromkeys(relation.to_name for relation in unresolved_relations) + # A path target (``./``, ``../``, ``/``) names a file relative to the note + # that carries it, so it is keyed by that note's path as well; identity + # targets stay keyed by text alone and resolve once for every source. + source_paths = ( + await self._source_paths(session, unresolved_relations) + if any(is_path_target(relation.to_name) for relation in unresolved_relations) + else {} ) - resolved_targets_by_link_text = ( + requests = list( + dict.fromkeys( + _target_request(relation, source_paths) for relation in unresolved_relations + ) + ) + resolved_targets_by_request = ( await self.target_resolver.resolve_relation_targets( - target_names, + requests, session=session, ) - if target_names + if requests else {} ) @@ -300,7 +321,7 @@ async def resolve_relations( f"from_id={relation.from_id} " f"to_name={relation.to_name}" ) - resolved_entity = resolved_targets_by_link_text[relation.to_name] + resolved_entity = resolved_targets_by_request[_target_request(relation, source_paths)] if resolved_entity is None or resolved_entity.id == relation.from_id: continue @@ -406,6 +427,18 @@ async def resolve_relations( return affected_entity_ids +def _target_request( + relation: UnresolvedRelation, + source_paths: Mapping[EntityId, str], +) -> RelationTargetRequest: + return RelationTargetRequest( + link_text=relation.to_name, + source_path=( + source_paths.get(relation.from_id) if is_path_target(relation.to_name) else None + ), + ) + + @dataclass(frozen=True, slots=True) class ResolveRelationsJobRequest: """Queue-neutral request shape for resolving one project's forward references.""" diff --git a/src/basic_memory/markdown/entity_parser.py b/src/basic_memory/markdown/entity_parser.py index a71fc97c6..a531ddcc2 100644 --- a/src/basic_memory/markdown/entity_parser.py +++ b/src/basic_memory/markdown/entity_parser.py @@ -14,7 +14,7 @@ from markdown_it import MarkdownIt from basic_memory.markdown.plugins import observation_plugin, relation_plugin -from basic_memory.markdown.path_links import markdown_link_target +from basic_memory.markdown.path_links import markdown_link_path from basic_memory.markdown.schemas import ( EntityFrontmatter, EntityMarkdown, @@ -155,7 +155,7 @@ class EntityContent: relations: list[Relation] = field(default_factory=list) -def parse(content: str, *, source_path: str | None = None) -> EntityContent: +def parse(content: str) -> EntityContent: """Parse markdown content into EntityMarkdown.""" # Parse content for observations and relations using markdown-it @@ -165,16 +165,16 @@ def parse(content: str, *, source_path: str | None = None) -> EntityContent: if content: for token in md.parse(content): # MarkdownIt owns link syntax, including escapes, reference links and - # code exclusion. Rooted targets retain exact project-path semantics - # through deferred resolution without changing the authored body. - if source_path is not None: - for child in token.children or []: - if child.type == "link_open": - href = child.attrGet("href") - assert isinstance(href, str) - target = markdown_link_target(href, source_path) if href else None - if target is not None: - relations.append(Relation(type="links_to", target=target)) + # code exclusion. A Markdown link is recorded as the path its author + # wrote; it resolves against the note's own path later, like any other + # relation, so parsing needs no idea of where the bytes came from. + for child in token.children or []: + if child.type == "link_open": + href = child.attrGet("href") + assert isinstance(href, str) + target = markdown_link_path(href) if href else None + if target is not None: + relations.append(Relation(type="links_to", target=target)) # check for observations and relations if token.meta: if "observation" in token.meta: @@ -361,16 +361,7 @@ async def parse_markdown_content( or (isinstance(semantic_setting, str) and semantic_setting.lower() == "false") ) entity_content = ( - parse( - post.content, - source_path=( - file_path.relative_to(self.base_path).as_posix() - if file_path.is_absolute() - else file_path.as_posix() - ), - ) - if parse_semantics - else EntityContent(content=post.content) + parse(post.content) if parse_semantics else EntityContent(content=post.content) ) # The parser reports only a qualifier the author plainly meant: an unknown kind, diff --git a/src/basic_memory/markdown/path_links.py b/src/basic_memory/markdown/path_links.py index f7aaff3dc..4c103fc5c 100644 --- a/src/basic_memory/markdown/path_links.py +++ b/src/basic_memory/markdown/path_links.py @@ -1,11 +1,36 @@ -"""Project-local targets carried by ordinary Markdown links.""" +"""Project-local file targets: Markdown links and path-shaped wikilinks. + +A path target names one file by where it sits, relative to the note that carries +it. It never resolves through a title, a permalink, a filename alias or another +project. The parser records the author's spelling; resolution turns it into a +project-root path once the note's own path is known, which is a database fact and +not a filesystem one. +""" from pathlib import PurePosixPath from urllib.parse import unquote, urlsplit +PATH_TARGET_PREFIXES = ("/", "./", "../") + + +def is_path_target(target: str) -> bool: + """Whether a relation target names a project file by path rather than identity. + + Rooted (``/``) and explicitly relative (``./``, ``../``) spellings are paths in + Markdown links and wikilinks alike. + """ + return target.startswith(PATH_TARGET_PREFIXES) + -def markdown_link_target(href: str, source_path: str) -> str | None: - """Return a project-root path, excluding URLs and paths escaping the project.""" +def markdown_link_path(href: str) -> str | None: + """Return the authored project path of a Markdown link, or None for anything else. + + URLs, ``mailto:`` and ``file:`` links, fragment-only links, and paths carrying + backslashes or null bytes are prose, not relations. Percent-encoding is decoded + and the query and fragment are dropped. The result keeps the author's spelling + relative to the note; a bare relative path is marked ``./`` so the stored + target says it is a path and not a title. + """ try: parsed = urlsplit(href) except ValueError: @@ -14,10 +39,27 @@ def markdown_link_target(href: str, source_path: str) -> str | None: if parsed.scheme or parsed.netloc or not parsed.path: return None path = unquote(parsed.path) - if "\\" in path or "\x00" in path: + if "\\" in path or "\x00" in path or path.endswith("/"): + # Backslashes and null bytes are not project paths; a trailing slash + # names a folder, and only files carry relations. + return None + return path if is_path_target(path) else f"./{path}" + + +def resolve_project_path(target: str, source_path: str | None) -> str | None: + """Resolve a path target against the note that carries it, as a project-root path. + + ``source_path`` is the note's own project path, such as ``notes/source.md``. + Rooted targets ignore it. Relative targets resolve against its folder, or + against the project root when no source is known. ``.`` and ``..`` segments + collapse, and a target that climbs past the root names no project file. + """ + if not is_path_target(target): return None - parts = [] if path.startswith("/") else list(PurePosixPath(source_path).parent.parts) - for part in path.split("/"): + parts: list[str] = [] + if not target.startswith("/") and source_path: + parts = list(PurePosixPath(source_path).parent.parts) + for part in target.split("/"): if part in {"", "."}: continue if part == "..": diff --git a/src/basic_memory/services/bulk_link_resolver.py b/src/basic_memory/services/bulk_link_resolver.py index 62547c657..8ecfc260e 100644 --- a/src/basic_memory/services/bulk_link_resolver.py +++ b/src/basic_memory/services/bulk_link_resolver.py @@ -8,6 +8,8 @@ from sqlalchemy.ext.asyncio import AsyncSession from basic_memory.config import BasicMemoryConfig +from basic_memory.indexing.models import RelationTargetRequest +from basic_memory.markdown.path_links import is_path_target, resolve_project_path from basic_memory.models import Entity, Project from basic_memory.repository.entity_repository import EntityRepository, file_path_alias from basic_memory.repository.project_repository import ProjectRepository @@ -30,22 +32,37 @@ class RelationTargetReference: original: str identifier: str explicitly_qualified: bool + # The note a path target is relative to; identity targets carry none. + source_path: str | None = None @classmethod - def parse(cls, link_text: str) -> "RelationTargetReference": + def parse(cls, link_text: str, source_path: str | None = None) -> "RelationTargetReference": """Normalize wikilink syntax once for the whole bulk-resolution pass.""" - if link_text.startswith("/"): - return cls(original=link_text, identifier=link_text, explicitly_qualified=False) clean_text, _ = normalize_link_text(link_text) + if is_path_target(clean_text): + return cls( + original=link_text, + identifier=clean_text, + explicitly_qualified=False, + source_path=source_path, + ) return cls( original=link_text, identifier=normalize_project_reference(clean_text), explicitly_qualified="::" in clean_text, ) + @classmethod + def from_request(cls, request: RelationTargetRequest) -> "RelationTargetReference": + return cls.parse(request.link_text, request.source_path) + + @property + def is_path(self) -> bool: + return is_path_target(self.identifier) + def project_path(self) -> tuple[str | None, str]: """Return a possible project prefix and its remaining target path.""" - if "/" not in self.identifier: + if self.is_path or "/" not in self.identifier: return None, self.identifier project_prefix, remainder = self.identifier.split("/", 1) @@ -208,10 +225,11 @@ def resolve(self, target: RelationTargetReference) -> Entity | None: """Resolve one parsed target without additional I/O.""" current_index = self.entity_indexes[self.current_project_id] - # Rooted Markdown targets are file identities, never title/permalink or - # cross-project guesses, including while their target is still absent. - if target.identifier.startswith("/"): - return current_index.by_file_path.get(target.identifier[1:]) + # Path targets are file identities relative to their source note, never + # title, permalink or cross-project guesses, including while absent. + if target.is_path: + project_path = resolve_project_path(target.identifier, target.source_path) + return current_index.by_file_path.get(project_path[1:]) if project_path else None try: external_id = str(uuid_mod.UUID(target.identifier)) @@ -332,13 +350,14 @@ class BulkLinkResolver: async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> dict[str, Entity | None]: + ) -> dict[RelationTargetRequest, Entity | None]: """Resolve unique relation targets with I/O bounded by referenced projects.""" + unique_requests = tuple(dict.fromkeys(requests)) targets = tuple( - RelationTargetReference.parse(link_text) for link_text in dict.fromkeys(link_texts) + RelationTargetReference.from_request(request) for request in unique_requests ) if not targets: return {} @@ -350,4 +369,7 @@ async def resolve_relation_targets( app_config=self.app_config, session=session, ) - return {target.original: snapshot.resolve(target) for target in targets} + return { + request: snapshot.resolve(target) + for request, target in zip(unique_requests, targets, strict=True) + } diff --git a/src/basic_memory/services/link_resolver.py b/src/basic_memory/services/link_resolver.py index ead06388a..76ea1b1df 100644 --- a/src/basic_memory/services/link_resolver.py +++ b/src/basic_memory/services/link_resolver.py @@ -8,6 +8,7 @@ from basic_memory import db from basic_memory.config import BasicMemoryConfig +from basic_memory.markdown.path_links import is_path_target, resolve_project_path from basic_memory.models import Entity, Project from basic_memory.repository.entity_repository import EntityRepository from basic_memory.repository.project_repository import ProjectRepository @@ -135,6 +136,10 @@ async def resolve_entity( repository. This is the target-project contract used by entity read and mutation flows. """ clean_text, _ = self._normalize_link_text(identifier) + if is_path_target(clean_text): + return await self._resolve_path_target( + clean_text, source_path, load_relations=load_relations, session=session + ) async with db.scoped_session(self.session_maker, session) as active_session: try: @@ -183,16 +188,12 @@ async def resolve_link( """ logger.trace(f"Resolving link: {link_text} (source: {source_path})") - # Markdown hrefs are normalized to project-root paths by the parser. - # They must not fall through to aliases, titles or another project. - if link_text.startswith("/"): - async with db.scoped_session(self.session_maker, session) as active_session: - return await self.entity_repository.get_by_file_path( - active_session, link_text[1:], load_relations=load_relations - ) - # Clean link text and extract any alias clean_text, alias = self._normalize_link_text(link_text) + if is_path_target(clean_text): + return await self._resolve_path_target( + clean_text, source_path, load_relations=load_relations, session=session + ) explicit_project_reference = "::" in clean_text clean_text = normalize_project_reference(clean_text) @@ -283,6 +284,27 @@ async def resolve_link( load_relations=load_relations, ) + async def _resolve_path_target( + self, + target: str, + source_path: Optional[str], + *, + load_relations: bool, + session: AsyncSession | None, + ) -> Optional[Entity]: + """Resolve a path target (``/``, ``./``, ``../``) to the one file it names. + + The path is taken relative to the note that carries the link. It never falls + through to titles, permalinks, filename aliases or another project. + """ + project_path = resolve_project_path(target, source_path) + if project_path is None: + return None + async with db.scoped_session(self.session_maker, session) as active_session: + return await self.entity_repository.get_by_file_path( + active_session, project_path[1:], load_relations=load_relations + ) + def _normalize_link_text(self, link_text: str) -> Tuple[str, Optional[str]]: """Normalize link text and extract alias if present. diff --git a/src/basic_memory/services/note_preparation.py b/src/basic_memory/services/note_preparation.py index 26aad1a84..852c01fb5 100644 --- a/src/basic_memory/services/note_preparation.py +++ b/src/basic_memory/services/note_preparation.py @@ -29,6 +29,7 @@ _coerce_to_string, normalize_frontmatter_metadata, ) +from basic_memory.markdown.path_links import is_path_target, resolve_project_path from basic_memory.markdown.utils import schema_to_markdown from basic_memory.models import Entity from basic_memory.repository import ( @@ -953,10 +954,15 @@ async def resolve_deferred_self_relation( entity: Entity, session: AsyncSession | None = None, ) -> Entity | None: - # Background resolution excludes self-edges, so exact Markdown paths must - # resolve here before wikilink alias parsing can reinterpret filename bytes. - if target.startswith("/"): - return entity if target[1:] == entity.file_path else None + # Background resolution excludes self-edges, so path targets must resolve + # here, against this note's own path, before wikilink alias parsing can + # reinterpret filename bytes. + if is_path_target(target): + return ( + entity + if resolve_project_path(target, entity.file_path) == f"/{entity.file_path}" + else None + ) clean_target = target.strip() if clean_target.startswith("[[") and clean_target.endswith("]]"): clean_target = clean_target[2:-2].strip() diff --git a/test-int/mcp/test_markdown_path_relations_integration.py b/test-int/mcp/test_markdown_path_relations_integration.py index e187055f2..001596e1f 100644 --- a/test-int/mcp/test_markdown_path_relations_integration.py +++ b/test-int/mcp/test_markdown_path_relations_integration.py @@ -6,6 +6,7 @@ from fastmcp import Client from basic_memory import db +from basic_memory.indexing.models import RelationTargetRequest from basic_memory.repository.entity_repository import EntityRepository from basic_memory.repository.relation_repository import RelationRepository from basic_memory.services.bulk_link_resolver import BulkLinkResolver @@ -15,10 +16,14 @@ async def test_markdown_paths_are_indexed_and_resolved_exactly( mcp_server, app, app_config, test_project, engine_factory ): - body = "See [target](../targets/Guide.md#details) and [web](https://example.com).\n" + body = ( + "See [target](../targets/Guide.md#details), [[../targets/Sibling.md]] " + "and [web](https://example.com).\n" + ) async with Client(mcp_server) as client: for title, directory, content in [ ("Guide", "targets", "# Guide\n\n## Details\nContent."), + ("Sibling", "targets", "# Sibling"), ("Source", "notes", body), ]: result = await client.call_tool( @@ -37,18 +42,29 @@ async def test_markdown_paths_are_indexed_and_resolved_exactly( relations = RelationRepository(project_id=test_project.id) async with db.scoped_session(session_maker) as session: target = await entities.get_by_file_path(session, "targets/Guide.md") - assert target is not None + sibling = await entities.get_by_file_path(session, "targets/Sibling.md") + assert target is not None and sibling is not None edges = await relations.find_by_type(session, "links_to") - assert [edge.to_name for edge in edges] == ["/targets/Guide.md"] - assert edges[0].to_id == target.id + # The graph keeps the author's spelling; both link kinds are paths here. + assert sorted(edge.to_name for edge in edges) == [ + "../targets/Guide.md", + "../targets/Sibling.md", + ] + assert {edge.to_name: edge.to_id for edge in edges} == { + "../targets/Guide.md": target.id, + "../targets/Sibling.md": sibling.id, + } + from_source = RelationTargetRequest("../targets/Guide.md", "notes/Source.md") + by_permalink = RelationTargetRequest("/Guide") + wrong_case = RelationTargetRequest("/targets/guide.md") resolved = await BulkLinkResolver(entities, app_config).resolve_relation_targets( - [edges[0].to_name, "/Guide", "/targets/guide.md"], session=session + [from_source, by_permalink, wrong_case], session=session ) - resolved_target = resolved[edges[0].to_name] + resolved_target = resolved[from_source] assert resolved_target is not None assert resolved_target.id == target.id - assert resolved["/Guide"] is None - assert resolved["/targets/guide.md"] is None + assert resolved[by_permalink] is None + assert resolved[wrong_case] is None assert body.strip() in (Path(test_project.path) / "notes" / "Source.md").read_text() @@ -77,5 +93,5 @@ async def test_markdown_self_link_is_resolved_when_written( session, "links_to" ) assert len(edges) == 1 - assert edges[0].to_name == "/notes/Self.md" + assert edges[0].to_name == "./Self.md" assert edges[0].to_id == source.id diff --git a/tests/index/test_local_project_index.py b/tests/index/test_local_project_index.py index 6dd7d1d3c..44fa410aa 100644 --- a/tests/index/test_local_project_index.py +++ b/tests/index/test_local_project_index.py @@ -54,6 +54,7 @@ ProjectIndexMoveRun, StoreProjectIndexMaintenanceRunner, ) +from basic_memory.indexing.models import RelationTargetRequest from basic_memory.indexing.relation_resolution import ( RepositoryRelationResolutionRuntime, ResolvedRelationTarget, @@ -2668,11 +2669,11 @@ async def clear_pending_search_refreshes( class RuntimeFactoryLinkResolver: async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> Mapping[str, ResolvedRelationTarget | None]: - return {link_text: None for link_text in link_texts} + ) -> Mapping[RelationTargetRequest, ResolvedRelationTarget | None]: + return {request: None for request in requests} class RuntimeFactoryEntityService: diff --git a/tests/indexing/test_batch_indexer.py b/tests/indexing/test_batch_indexer.py index 4f983a019..4c9c709bc 100644 --- a/tests/indexing/test_batch_indexer.py +++ b/tests/indexing/test_batch_indexer.py @@ -1512,9 +1512,9 @@ async def test_batch_indexer_uses_exact_bulk_resolution_for_deferred_relations( original_resolve_targets = target_resolver_type.resolve_relation_targets seen_target_batches: list[tuple[str, ...]] = [] - async def spy_resolve_targets(self, link_texts, *, session): - seen_target_batches.append(tuple(link_texts)) - return await original_resolve_targets(self, link_texts, session=session) + async def spy_resolve_targets(self, requests, *, session): + seen_target_batches.append(tuple(request.link_text for request in requests)) + return await original_resolve_targets(self, requests, session=session) monkeypatch.setattr(target_resolver_type, "resolve_relation_targets", spy_resolve_targets) diff --git a/tests/indexing/test_relation_resolution.py b/tests/indexing/test_relation_resolution.py index c799eec83..deb715938 100644 --- a/tests/indexing/test_relation_resolution.py +++ b/tests/indexing/test_relation_resolution.py @@ -23,7 +23,7 @@ resolve_project_index_completion_relations, resolve_project_relations, ) -from basic_memory.indexing.models import IndexFileJobStatus +from basic_memory.indexing.models import IndexFileJobStatus, RelationTargetRequest from basic_memory.models import Entity from basic_memory.repository.relation_repository import ( PendingRelationSearchRefresh, @@ -84,6 +84,7 @@ class FakeRelation: class FakeResolvedEntity: id: int title: str + file_path: str = "" @property def external_id(self) -> str: @@ -222,16 +223,18 @@ class StubLinkResolver: def __init__(self, targets: dict[str, FakeResolvedEntity]) -> None: self.targets = targets self.calls: list[tuple[str, bool]] = [] + self.requests: list[RelationTargetRequest] = [] async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> Mapping[str, FakeResolvedEntity | None]: + ) -> Mapping[RelationTargetRequest, FakeResolvedEntity | None]: assert isinstance(session, FakeSession) - self.calls.extend((link_text, True) for link_text in link_texts) - return {link_text: self.targets.get(link_text) for link_text in link_texts} + self.requests.extend(requests) + self.calls.extend((request.link_text, True) for request in requests) + return {request: self.targets.get(request.link_text) for request in requests} class StubEntityIndexer: @@ -283,11 +286,12 @@ def build_repository_runtime( target_resolver: StubLinkResolver, entity_indexer: StubEntityIndexer, note_contents: Sequence[FakeNoteContent] = (), + entity_repository: StubEntityRepository | None = None, ) -> RepositoryRelationResolutionRuntime: return RepositoryRelationResolutionRuntime( session_maker=cast(async_sessionmaker[AsyncSession], FakeSession), relation_repository=relation_repository, - entity_repository=StubEntityRepository(), + entity_repository=entity_repository or StubEntityRepository(), note_content_repository=StubNoteContentRepository(note_contents), target_resolver=target_resolver, entity_indexer=entity_indexer, @@ -498,6 +502,54 @@ async def test_resolution_loop_is_bounded_by_max_passes() -> None: assert runtime.resolve_calls == 3 +@pytest.mark.asyncio +async def test_path_targets_are_resolved_per_source_note() -> None: + """The same authored path from two notes names two files; a title resolves once for both.""" + repo = StubRelationRepository( + [ + [ + FakeRelation(id=1, from_id=10, to_name="./Guide.md"), + FakeRelation(id=2, from_id=11, to_name="./Guide.md"), + FakeRelation(id=3, from_id=10, to_name="Shared Title"), + FakeRelation(id=4, from_id=11, to_name="Shared Title"), + ], + [], + ] + ) + guides = { + RelationTargetRequest("./Guide.md", "a/Source A.md"): FakeResolvedEntity(20, "Guide A"), + RelationTargetRequest("./Guide.md", "b/Source B.md"): FakeResolvedEntity(21, "Guide B"), + RelationTargetRequest("Shared Title"): FakeResolvedEntity(22, "Shared Title"), + } + + class SourceAwareLinkResolver(StubLinkResolver): + @override + async def resolve_relation_targets( + self, + requests: Sequence[RelationTargetRequest], + *, + session: AsyncSession, + ) -> Mapping[RelationTargetRequest, FakeResolvedEntity | None]: + self.requests.extend(requests) + return {request: guides.get(request) for request in requests} + + link_resolver = SourceAwareLinkResolver({}) + sources = StubEntityRepository() + sources.entities = { + 10: cast(Entity, FakeResolvedEntity(10, "Source A", file_path="a/Source A.md")), + 11: cast(Entity, FakeResolvedEntity(11, "Source B", file_path="b/Source B.md")), + } + runtime = build_repository_runtime( + repo, link_resolver, StubEntityIndexer(), entity_repository=sources + ) + + assert await runtime.resolve_relations() == {10, 11} + + assert link_resolver.requests == list(guides) + written = sorted((write.relation_id, write.target_id) for write in repo.write_batches[0]) + assert written == [(1, 20), (2, 21), (3, 22), (4, 22)] + + @pytest.mark.asyncio async def test_project_relation_resolution_uses_repository_runtime_and_counts_remaining() -> None: repo = StubRelationRepository( @@ -793,15 +845,19 @@ def __init__( @override async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> Mapping[str, FakeResolvedEntity | None]: + ) -> Mapping[RelationTargetRequest, FakeResolvedEntity | None]: assert isinstance(session, FakeSession) - self.calls.extend((link_text, True) for link_text in link_texts) + self.calls.extend((request.link_text, True) for request in requests) return { - link_text: None if link_text in self.ambiguous else self.targets.get(link_text) - for link_text in link_texts + request: ( + None + if request.link_text in self.ambiguous + else self.targets.get(request.link_text) + ) + for request in requests } repo = StubRelationRepository( diff --git a/tests/indexing/test_relation_search_refresh_retry.py b/tests/indexing/test_relation_search_refresh_retry.py index 38d968184..762327ad4 100644 --- a/tests/indexing/test_relation_search_refresh_retry.py +++ b/tests/indexing/test_relation_search_refresh_retry.py @@ -19,6 +19,7 @@ NoteContentRepository, ) from basic_memory.repository.relation_repository import RelationRepository +from basic_memory.indexing.models import RelationTargetRequest from basic_memory.schemas.search import SearchItemType @@ -31,13 +32,13 @@ def __init__(self, targets: Mapping[str, Entity]) -> None: async def resolve_relation_targets( self, - link_texts: Sequence[str], + requests: Sequence[RelationTargetRequest], *, session: AsyncSession, - ) -> Mapping[str, Entity | None]: + ) -> Mapping[RelationTargetRequest, Entity | None]: del session - self.calls += len(link_texts) - return {link_text: self.targets.get(link_text) for link_text in link_texts} + self.calls += len(requests) + return {request: self.targets.get(request.link_text) for request in requests} @pytest.mark.asyncio diff --git a/tests/markdown/test_path_links.py b/tests/markdown/test_path_links.py index 273c17ec8..f71da2d9d 100644 --- a/tests/markdown/test_path_links.py +++ b/tests/markdown/test_path_links.py @@ -1,32 +1,81 @@ -"""Ordinary Markdown links retain exact project-local path semantics.""" +"""Ordinary Markdown links are recorded as authored paths and resolved against their note.""" import pytest from basic_memory.markdown.entity_parser import EntityParser, parse -from basic_memory.markdown.path_links import markdown_link_target +from basic_memory.markdown.path_links import ( + is_path_target, + markdown_link_path, + resolve_project_path, +) @pytest.mark.parametrize( ("href", "expected"), [ - ("../Guide%20One.md#section", "/Guide One.md"), - ("same.md", "/notes/same.md"), - ("./nested/../same.md", "/notes/same.md"), + ("../Guide%20One.md#section", "../Guide One.md"), + ("same.md", "./same.md"), + ("./nested/../same.md", "./nested/../same.md"), ("/root.md", "/root.md"), - ("../../outside.md", None), + ("../../outside.md", "../../outside.md"), ("https://example.com/note.md", None), ("//example.com/note.md", None), ("mailto:me@example.com", None), ("file:///tmp/note.md", None), ("https://[broken", None), ("#section", None), - ("../", None), ("bad%00.md", None), ("bad%5Cpath.md", None), + ("./", None), + ("../", None), + ("docs/", None), + ], +) +def test_markdown_link_keeps_the_authored_path(href, expected): + """The parser records where the author pointed; resolution happens later.""" + assert markdown_link_path(href) == expected + + +@pytest.mark.parametrize( + ("target", "source_path", "expected"), + [ + ("../Guide One.md", "notes/source.md", "/Guide One.md"), + ("./same.md", "notes/source.md", "/notes/same.md"), + ("./nested/../same.md", "notes/source.md", "/notes/same.md"), + ("/root.md", "notes/source.md", "/root.md"), + ("/root.md", None, "/root.md"), + ("./same.md", None, "/same.md"), + ("./same.md", "source.md", "/same.md"), + ("../../outside.md", "notes/source.md", None), + ("../same.md", None, None), + ("../", "notes/source.md", None), + ("Guide One", "notes/source.md", None), + ], +) +def test_path_targets_resolve_against_the_note_that_carries_them(target, source_path, expected): + assert resolve_project_path(target, source_path) == expected + + +@pytest.mark.parametrize( + ("target", "expected"), + [("/root.md", True), ("./same.md", True), ("../up.md", True), ("Guide", False), ("a/b", False)], +) +def test_path_targets_are_rooted_or_explicitly_relative(target, expected): + assert is_path_target(target) is expected + + +@pytest.mark.parametrize( + ("href", "source_path", "expected"), + [ + ("../Guide%20One.md#section", "notes/source.md", "/Guide One.md"), + ("https://example.com/note.md", "notes/source.md", None), + ("../../outside.md", "notes/source.md", None), ], ) -def test_markdown_target_is_bounded_to_project(href, expected): - assert markdown_link_target(href, "notes/source.md") == expected +def test_authoring_then_resolution_names_one_project_file(href, source_path, expected): + path = markdown_link_path(href) + resolved = resolve_project_path(path, source_path) if path is not None else None + assert resolved == expected def test_markdown_parser_uses_real_links_without_rewriting_content(): @@ -36,11 +85,11 @@ def test_markdown_parser_uses_real_links_without_rewriting_content(): [ref]: next.md """ - parsed = parse(content, source_path="notes/source.md") + parsed = parse(content) assert parsed.content == content assert [(relation.type, relation.target) for relation in parsed.relations] == [ - ("links_to", "/Guide One.md"), - ("links_to", "/notes/next.md"), + ("links_to", "../Guide One.md"), + ("links_to", "./next.md"), ("links_to", "Existing Wiki"), ] @@ -53,3 +102,16 @@ async def test_remote_content_parsing_respects_semantic_opt_out(tmp_path): parsed = await parser.parse_markdown_content(tmp_path / "absent.md", content) assert parsed.content == body assert parsed.relations == [] + + +@pytest.mark.asyncio +async def test_parsing_needs_no_filesystem_location_for_the_note(tmp_path): + """Content read from remote storage parses under any path, inside the root or not.""" + parser = EntityParser(tmp_path / "project") + content = "[guide](../guides/Guide.md) and [[../guides/Other.md]]" + for file_path in (tmp_path / "elsewhere" / "tmp.md", tmp_path / "project" / "notes" / "a.md"): + parsed = await parser.parse_markdown_content(file_path, content) + assert [relation.target for relation in parsed.relations] == [ + "../guides/Guide.md", + "../guides/Other.md", + ] diff --git a/tests/services/test_bulk_link_resolver.py b/tests/services/test_bulk_link_resolver.py index b33d419ff..fcf2edeb7 100644 --- a/tests/services/test_bulk_link_resolver.py +++ b/tests/services/test_bulk_link_resolver.py @@ -7,6 +7,7 @@ from basic_memory import db from basic_memory.config import BasicMemoryConfig +from basic_memory.indexing.models import RelationTargetRequest from basic_memory.models import Entity, Project from basic_memory.repository import EntityRepository, ProjectRepository from basic_memory.services.bulk_link_resolver import ( @@ -119,7 +120,7 @@ async def test_bulk_resolution_matches_regular_strict_resolution( async with db.scoped_session(session_maker) as session: bulk_results = await bulk_resolver.resolve_relation_targets( - link_texts, + [RelationTargetRequest(link_text) for link_text in link_texts], session=session, ) for link_text in link_texts[:-1]: @@ -129,7 +130,7 @@ async def test_bulk_resolution_matches_regular_strict_resolution( load_relations=False, session=session, ) - bulk_result = bulk_results[link_text] + bulk_result = bulk_results[RelationTargetRequest(link_text)] assert (bulk_result.id if bulk_result else None) == ( regular_result.id if regular_result else None ) @@ -142,7 +143,7 @@ async def test_bulk_resolution_matches_regular_strict_resolution( session=session, ) - assert bulk_results["Core Service"] is None + assert bulk_results[RelationTargetRequest("Core Service")] is None @pytest.mark.asyncio @@ -195,17 +196,25 @@ async def test_bulk_resolution_normalizes_file_paths( resolver = BulkLinkResolver(entity_repository, app_config) async with db.scoped_session(session_maker) as session: - results = await resolver.resolve_relation_targets( - [ - "./assets//image.png", - "docs/Guide", - "alpha_note", - "alpha-note", - "ALPHA-NOTE.MD", - "école-note", - ], - session=session, - ) + results = { + request.link_text: entity + for request, entity in ( + await resolver.resolve_relation_targets( + [ + RelationTargetRequest(link_text) + for link_text in ( + "./assets//image.png", + "docs/Guide", + "alpha_note", + "alpha-note", + "ALPHA-NOTE.MD", + "école-note", + ) + ], + session=session, + ) + ).items() + } resolved_image = results["./assets//image.png"] assert resolved_image is not None @@ -326,14 +335,22 @@ async def test_bulk_resolution_routes_cross_project_targets( project_repository=project_repository, ) async with db.scoped_session(session_maker) as session: - results = await resolver.resolve_relation_targets( - [ - "other project::Cross Project Note", - "Other Project/docs/cross-project-note", - "missing::Cross Project Note", - ], - session=session, - ) + results = { + request.link_text: entity + for request, entity in ( + await resolver.resolve_relation_targets( + [ + RelationTargetRequest(link_text) + for link_text in ( + "other project::Cross Project Note", + "Other Project/docs/cross-project-note", + "missing::Cross Project Note", + ) + ], + session=session, + ) + ).items() + } explicit_result = results["other project::Cross Project Note"] path_result = results["Other Project/docs/cross-project-note"] @@ -365,4 +382,73 @@ async def test_bulk_resolution_requires_the_current_project_to_exist( async with db.scoped_session(session_maker) as session: with pytest.raises(RuntimeError, match="Current project 999999 does not exist"): - await resolver.resolve_relation_targets(["Target"], session=session) + await resolver.resolve_relation_targets( + [RelationTargetRequest("Target")], session=session + ) + + +@pytest.mark.asyncio +async def test_path_targets_resolve_from_the_note_that_carries_them( + entity_repository: EntityRepository, + test_project: Project, + session_maker, + app_config: BasicMemoryConfig, +) -> None: + """The same authored path names a different file from each source note.""" + now = datetime.now(timezone.utc) + entities = [ + Entity( + title=title, + note_type="note", + content_type="text/markdown", + file_path=file_path, + permalink=permalink, + created_at=now, + updated_at=now, + project_id=test_project.id, + ) + for title, file_path, permalink in ( + ("Alpha Guide", "alpha/Guide.md", "alpha/guide"), + ("Beta Guide", "beta/Guide.md", "beta/guide"), + ("Shared", "Shared.md", "shared"), + ) + ] + async with db.scoped_session(session_maker) as session: + for entity in entities: + await entity_repository.add(session, entity) + alpha_guide, beta_guide, shared = entities + from_alpha = RelationTargetRequest("./Guide.md", source_path="alpha/notes.md") + from_beta = RelationTargetRequest("./Guide.md", source_path="beta/notes.md") + up_from_alpha = RelationTargetRequest("../Shared.md", source_path="alpha/notes.md") + wikilink_from_beta = RelationTargetRequest("[[../Shared.md|the shared note]]", "beta/notes.md") + rooted = RelationTargetRequest("/beta/Guide.md", source_path="alpha/notes.md") + escaping = RelationTargetRequest("../../Shared.md", source_path="alpha/notes.md") + no_source = RelationTargetRequest("../Shared.md") + by_title = RelationTargetRequest("./guide", source_path="alpha/notes.md") + + async with db.scoped_session(session_maker) as session: + results = await BulkLinkResolver(entity_repository, app_config).resolve_relation_targets( + [ + from_alpha, + from_beta, + up_from_alpha, + wikilink_from_beta, + rooted, + escaping, + no_source, + by_title, + ], + session=session, + ) + + assert {request: entity.id if entity else None for request, entity in results.items()} == { + from_alpha: alpha_guide.id, + from_beta: beta_guide.id, + up_from_alpha: shared.id, + wikilink_from_beta: shared.id, + rooted: beta_guide.id, + escaping: None, + no_source: None, + # A path names a file exactly; it never falls back to a permalink or title. + by_title: None, + } diff --git a/tests/services/test_markdown_path_resolution.py b/tests/services/test_markdown_path_resolution.py index 4a0698f78..019bba91f 100644 --- a/tests/services/test_markdown_path_resolution.py +++ b/tests/services/test_markdown_path_resolution.py @@ -1,4 +1,4 @@ -"""Rooted path relations cannot resolve to semantic aliases.""" +"""Path targets resolve to one exact file, relative to the note that carries them.""" from datetime import datetime, timezone @@ -8,25 +8,69 @@ from basic_memory.models import Entity -@pytest.mark.asyncio -async def test_rooted_path_resolves_only_exact_file( - link_resolver, entity_repository, test_project, session_maker -): +async def _add(entity_repository, session_maker, test_project, file_path: str, title: str): now = datetime.now(timezone.utc) entity = Entity( - title="Guide", + title=title, note_type="note", content_type="text/markdown", - file_path="notes/Guide.md", - permalink="guide", + file_path=file_path, + permalink=file_path[:-3].lower().replace(" ", "-"), created_at=now, updated_at=now, project_id=test_project.id, ) async with db.scoped_session(session_maker) as session: await entity_repository.add(session, entity) + return entity + + +@pytest.mark.asyncio +async def test_rooted_path_resolves_only_exact_file( + link_resolver, entity_repository, test_project, session_maker +): + entity = await _add(entity_repository, session_maker, test_project, "notes/Guide.md", "Guide") resolved = await link_resolver.resolve_link("/notes/Guide.md") assert resolved is not None assert resolved.id == entity.id assert await link_resolver.resolve_link("/guide") is None assert await link_resolver.resolve_link("/notes/guide.md") is None + + +@pytest.mark.asyncio +async def test_relative_paths_resolve_from_the_source_note( + link_resolver, entity_repository, test_project, session_maker +): + guide = await _add(entity_repository, session_maker, test_project, "notes/Guide.md", "Guide") + shared = await _add(entity_repository, session_maker, test_project, "Shared.md", "Shared") + + sibling = await link_resolver.resolve_link("./Guide.md", source_path="notes/Source.md") + parent = await link_resolver.resolve_link("../Shared.md", source_path="notes/Source.md") + assert sibling is not None and sibling.id == guide.id + assert parent is not None and parent.id == shared.id + # The same spelling from another folder names another file, or none. + assert await link_resolver.resolve_link("./Guide.md", source_path="other/Source.md") is None + # Without a source, only the root is a base: `./` resolves there, `../` cannot. + root_shared = await link_resolver.resolve_link("./Shared.md") + assert root_shared is not None and root_shared.id == shared.id + assert await link_resolver.resolve_link("../Shared.md") is None + # Climbing past the project root names nothing. + assert ( + await link_resolver.resolve_link("../../Shared.md", source_path="notes/Source.md") is None + ) + + +@pytest.mark.asyncio +async def test_wikilinks_spelled_as_paths_follow_the_same_rule( + link_resolver, entity_repository, test_project, session_maker +): + shared = await _add(entity_repository, session_maker, test_project, "Shared.md", "Shared") + + resolved = await link_resolver.resolve_link( + "[[../Shared.md|the shared note]]", source_path="notes/Source.md" + ) + assert resolved is not None and resolved.id == shared.id + # A path never falls through to the title or permalink the file also answers to. + assert await link_resolver.resolve_link("./shared", source_path="notes/Source.md") is None + by_entity = await link_resolver.resolve_entity("../Shared.md", source_path="notes/Source.md") + assert by_entity is not None and by_entity.id == shared.id