From 011390d20fcd0da662e398cfa20c5868882b68ce Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 15 Sep 2026 22:05:56 -0500 Subject: [PATCH] fix(core): resolve Markdown path links against the note at resolution time A Markdown link was the only relation whose meaning depended on where the parser read the bytes: the parser derived the note's project path from the filesystem path with relative_to and turned the href into a rooted path at parse time. Content parsed from anywhere outside the project root raised (cloud reads a note from object storage into a temp file), and any other temporary location gave every link the wrong base. Wikilinks are recorded as authored and resolved later against database state. Path links now behave the same way: - The parser stores the path as the author wrote it: ../guides/Guide.md, ./same.md (a bare same.md gets the ./ mark so the stored target says it is a path and not a title), or a rooted /root.md. parse() takes no source path. - Both resolvers detect a path target (/, ./, ../) after wikilink normalization and resolve it against the note's own project path with resolve_project_path. Exact file only; no title, permalink, alias or cross-project fallback; a path that climbs past the root names nothing. Path-shaped wikilinks such as [[../x.md]] follow the same rule. - Background resolution keys targets by RelationTargetRequest(link_text, source_path). Identity targets carry no source and resolve once for every note; path targets are keyed by their source note, so ./Guide.md from two folders resolves to two files in one pass. Source paths are loaded with one find_by_ids call only when a batch contains a path target. - The write-time self-link check resolves the authored path against the note's own path. Existing relation rows for Markdown links are rewritten to the authored form on the note's next edit or reindex, as the docs already state. Refs #1514 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_019YW9ysxugGGBCNEGzsxtFV Signed-off-by: phernandez --- CHANGELOG.md | 11 ++ docs/MARKDOWN_RELATIONS.md | 12 +- src/basic_memory/indexing/models.py | 13 ++ .../indexing/relation_resolution.py | 51 +++++-- src/basic_memory/markdown/entity_parser.py | 35 ++--- src/basic_memory/markdown/path_links.py | 54 ++++++- .../services/bulk_link_resolver.py | 46 ++++-- src/basic_memory/services/link_resolver.py | 38 +++-- src/basic_memory/services/note_preparation.py | 14 +- ...est_markdown_path_relations_integration.py | 34 +++-- tests/index/test_local_project_index.py | 7 +- tests/indexing/test_batch_indexer.py | 6 +- tests/indexing/test_relation_resolution.py | 78 +++++++++-- .../test_relation_search_refresh_retry.py | 9 +- tests/markdown/test_path_links.py | 86 ++++++++++-- tests/services/test_bulk_link_resolver.py | 132 +++++++++++++++--- .../services/test_markdown_path_resolution.py | 60 ++++++-- 17 files changed, 550 insertions(+), 136 deletions(-) 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