diff --git a/src/basic_memory/cli/commands/cloud/project_sync.py b/src/basic_memory/cli/commands/cloud/project_sync.py index 94e84ed0a..99c364972 100644 --- a/src/basic_memory/cli/commands/cloud/project_sync.py +++ b/src/basic_memory/cli/commands/cloud/project_sync.py @@ -8,6 +8,7 @@ import os from datetime import datetime from enum import Enum +from pathlib import Path import typer from rich.console import Console @@ -17,8 +18,6 @@ from basic_memory.cli.commands.cloud.rclone_commands import ( RcloneError, SyncProject, - TransferDirection, - TransferPlan, get_bmignore_prune_filter_path, get_project_bisync_state, project_bisync, @@ -34,6 +33,12 @@ rclone_remote_exists, remote_name_for_workspace, ) +from basic_memory.cli.commands.cloud.transfer import TransferDirection, TransferPlan +from basic_memory.cli.commands.cloud.webdav import WebdavError +from basic_memory.cli.commands.cloud.webdav_transfer import ( + webdav_project_diff, + webdav_project_transfer, +) from basic_memory.cli.commands.command_utils import run_with_cleanup from basic_memory.cli.commands.routing import force_routing from basic_memory.config import BasicMemoryConfig, ConfigManager, ProjectEntry @@ -79,8 +84,8 @@ class ConflictStrategy(str, Enum): leaving the user to re-run with an explicit resolution — like git refusing to clobber local changes. - This is the Typer-facing enum; the engine in ``rclone_commands`` accepts the - same values as a ``ConflictStrategy`` Literal. ``_run_directional_transfer`` + This is the Typer-facing enum; both transfer engines accept the same values + as the ``ConflictStrategy`` Literal in ``transfer``. The orchestration bridges the two by passing ``on_conflict.value``. Keep the values in sync. """ @@ -213,6 +218,24 @@ async def _get_cloud_project(name: str, *, workspace_id: str | None = None) -> P return None +def _require_local_sync_path(name: str, config: BasicMemoryConfig) -> str: + """Resolve the local directory a project syncs against, or exit with guidance. + + Both transports need this: the rclone path passes it to rclone, the WebDAV + path walks it directly. + """ + sync_entry = config.projects.get(name) + # Support both new (path) and legacy (local_sync_path) configs + local_sync_path = (sync_entry.local_sync_path or sync_entry.path) if sync_entry else None + + if not local_sync_path or not os.path.isabs(local_sync_path): + console.print(f"[red]Error: Project '{name}' has no local sync path configured[/red]") + console.print(f"\nConfigure sync with: bm cloud sync-setup {name} ~/path/to/local") + raise typer.Exit(1) + + return local_sync_path + + def _get_sync_project( name: str, config: BasicMemoryConfig, @@ -227,14 +250,7 @@ def _get_sync_project( Returns (sync_project, local_sync_path). Exits if no local_sync_path configured. """ - sync_entry = config.projects.get(name) - # Support both new (path) and legacy (local_sync_path) configs - local_sync_path = (sync_entry.local_sync_path or sync_entry.path) if sync_entry else None - - if not local_sync_path or not os.path.isabs(local_sync_path): - console.print(f"[red]Error: Project '{name}' has no local sync path configured[/red]") - console.print(f"\nConfigure sync with: bm cloud sync-setup {name} ~/path/to/local") - raise typer.Exit(1) + local_sync_path = _require_local_sync_path(name, config) sync_project = SyncProject( name=project_data.name, @@ -451,6 +467,116 @@ def _print_conflict_abort(name: str, direction: TransferDirection, plan: Transfe ) +# Why the two transports word this differently: rclone reports files it could not +# read or hash, while the WebDAV transport reports files the service described +# without any validator it could compare. Same abort, different cause. +RCLONE_COMPARE_FAILURE = "rclone could not compare {count} file(s)" +WEBDAV_COMPARE_FAILURE = ( + "the cloud reported no comparable checksum or timestamp for {count} file(s)" +) + + +def _check_plan( + name: str, + direction: TransferDirection, + plan: TransferPlan, + on_conflict: ConflictStrategy, + *, + compare_failure: str, +) -> None: + """Apply the pre-transfer gates shared by both transports. + + Trigger: files that could not be compared, or conflicts with no chosen + resolution. + Why: comparing is the whole basis for a safe transfer — never guess, and + never silently pick a winner. + Outcome: abort before moving any bytes, listing what needs attention. + """ + if plan.errors: + console.print( + f"[red]{direction.capitalize()} aborted: " + f"{compare_failure.format(count=len(plan.errors))}[/red]" + ) + for path in plan.errors: + console.print(f" [red]![/red] {path}") + raise typer.Exit(1) + + if plan.conflicts and on_conflict is ConflictStrategy.fail: + _print_conflict_abort(name, direction, plan) + raise typer.Exit(1) + + +def _report_transfer_complete(name: str, direction: TransferDirection, plan: TransferPlan) -> None: + """Announce success and account for what was deliberately left alone.""" + console.print(f"[green]{name} {direction} completed successfully[/green]") + + # Without a sync baseline (see #862) we cannot tell an intentional delete + # from a file the other side simply never had, so deletions never sync. + if plan.dest_only: + kept_on = "local" if direction == "pull" else "cloud" + console.print( + f"[dim]{len(plan.dest_only)} file(s) exist only on {kept_on} and were left " + "untouched (deletions are not propagated).[/dim]" + ) + + +def _run_webdav_directional_transfer( + name: str, + direction: TransferDirection, + *, + config: BasicMemoryConfig, + workspace: WorkspaceInfo, + on_conflict: ConflictStrategy, + dry_run: bool, + verbose: bool, +) -> None: + """Run a Team-workspace push/pull over the cloud WebDAV surface. + + No `bm cloud setup`, no rclone remote, no storage credentials: the service + authorizes every request against the caller's access to this project, which + is the model a shared workspace needs (#1262). Conflict handling, additive + semantics, and the CLI's output are the same as the Personal path. + """ + with force_routing(cloud=True): + project_data = run_with_cleanup(_get_cloud_project(name, workspace_id=workspace.tenant_id)) + if not project_data: + console.print(f"[red]Error: Project '{name}' not found[/red]") + raise typer.Exit(1) + + local_root = Path(_require_local_sync_path(name, config)) + + # --- Detect before transferring --- + plan = run_with_cleanup( + webdav_project_diff( + project_data.name, + local_root, + direction, + workspace_id=workspace.tenant_id, + ) + ) + _check_plan(name, direction, plan, on_conflict, compare_failure=WEBDAV_COMPARE_FAILURE) + + # --- Transfer --- + arrow = "cloud -> local" if direction == "pull" else "local -> cloud" + console.print(f"[blue]{direction.capitalize()} {name} ({arrow})...[/blue]") + + run_with_cleanup( + webdav_project_transfer( + project_data.name, + local_root, + direction, + plan, + workspace_id=workspace.tenant_id, + strategy=on_conflict.value, + conflict_suffix=datetime.now().strftime("%Y%m%d-%H%M%S"), + dry_run=dry_run, + verbose=verbose, + ) + ) + + _report_transfer_complete(name, direction, plan) + + def _run_directional_transfer( name: str, direction: TransferDirection, @@ -463,20 +589,25 @@ def _run_directional_transfer( """Shared orchestration for `bm cloud push` / `bm cloud pull`. Detects conflicts first, then aborts (the default) or applies the chosen - resolution. Uses additive `rclone copy`, so it never deletes on the - destination — safe for Team workspaces and therefore not gated. - - Routes through the resolved workspace's own tenant-scoped rclone remote, so a - Team project reads/writes the right bucket (see #919). + resolution. Both transports are additive — neither ever deletes on the + destination — which is what makes these commands safe on Team workspaces. + + The transport depends on the workspace. Personal workspaces run `rclone copy` + against their own tenant-scoped remote, so the project reads/writes the right + bucket (see #919). Team workspaces go over WebDAV instead: storage + credentials are scoped to a whole tenant bucket and so cannot honor + per-project access, and minting them is owner-only, which left members unable + to run these commands at all (#1262). """ config = ConfigManager().config _require_cloud_credentials(config) try: - # --- Resolve the target workspace and its tenant-scoped remote --- - # Tigris credentials are bucket/tenant-scoped, so each workspace has its - # own rclone remote. Resolve which workspace this project belongs to - # (config or --workspace override) before touching any bucket. + # --- Resolve the target workspace and its transport --- + # The workspace decides both the transport and, on the Personal path, + # which tenant-scoped rclone remote to use. Resolve which workspace this + # project belongs to (config or --workspace override) before going near + # a bucket or the cloud's file surface. try: target_workspace = run_with_cleanup( _get_workspace_for_project(name, config, workspace_override=workspace) @@ -485,6 +616,25 @@ def _run_directional_transfer( console.print(f"[red]Error resolving workspace for project '{name}': {exc}[/red]") raise typer.Exit(1) + # Trigger: the resolved workspace is shared (an organization workspace). + # Why: the rclone path below needs tenant-wide storage credentials, which + # bypass the service's per-project access control and can only be minted + # by a workspace owner — every other member got a 403 pointing at a + # command that could never succeed (#1262). + # Outcome: Team transfers run over WebDAV, where the service applies + # per-project access to each request. Personal is untouched. + if target_workspace.workspace_type != "personal": + _run_webdav_directional_transfer( + name, + direction, + config=config, + workspace=target_workspace, + on_conflict=on_conflict, + dry_run=dry_run, + verbose=verbose, + ) + return + remote_name = remote_name_for_workspace( target_workspace.slug, is_default=target_workspace.is_default ) @@ -519,25 +669,7 @@ def _run_directional_transfer( # --- Detect before transferring --- plan = project_diff(sync_project, bucket_name, direction) - - # Trigger: rclone could not read/hash some files. - # Why: comparing is the whole basis for a safe transfer — never guess. - # Outcome: abort before moving any bytes. - if plan.errors: - console.print( - f"[red]{direction.capitalize()} aborted: rclone could not compare " - f"{len(plan.errors)} file(s)[/red]" - ) - for path in plan.errors: - console.print(f" [red]![/red] {path}") - raise typer.Exit(1) - - # Trigger: files differ on both sides and the user chose no resolution. - # Why: "no surprises" — never silently pick a winner. - # Outcome: list the conflicts and exit, like git refusing to clobber. - if plan.conflicts and on_conflict is ConflictStrategy.fail: - _print_conflict_abort(name, direction, plan) - raise typer.Exit(1) + _check_plan(name, direction, plan, on_conflict, compare_failure=RCLONE_COMPARE_FAILURE) # --- Transfer --- arrow = "cloud -> local" if direction == "pull" else "local -> cloud" @@ -559,18 +691,9 @@ def _run_directional_transfer( console.print(f"[red]{name} {direction} failed[/red]") raise typer.Exit(1) - console.print(f"[green]{name} {direction} completed successfully[/green]") + _report_transfer_complete(name, direction, plan) - # Without a sync baseline (see #862) we cannot tell an intentional delete - # from a file the other side simply never had, so deletions never sync. - if plan.dest_only: - kept_on = "local" if direction == "pull" else "cloud" - console.print( - f"[dim]{len(plan.dest_only)} file(s) exist only on {kept_on} and were left " - "untouched (deletions are not propagated).[/dim]" - ) - - except RcloneError as e: + except (RcloneError, WebdavError) as e: console.print(f"[red]{direction.capitalize()} error: {e}[/red]") raise typer.Exit(1) except typer.Exit: diff --git a/src/basic_memory/cli/commands/cloud/rclone_commands.py b/src/basic_memory/cli/commands/cloud/rclone_commands.py index 2244d743c..e0af3c92f 100644 --- a/src/basic_memory/cli/commands/cloud/rclone_commands.py +++ b/src/basic_memory/cli/commands/cloud/rclone_commands.py @@ -13,15 +13,22 @@ import re import subprocess from collections.abc import Sequence -from dataclasses import dataclass, field +from dataclasses import dataclass from functools import lru_cache -from pathlib import Path, PurePosixPath -from typing import Callable, Literal, Optional, Protocol +from pathlib import Path +from typing import Callable, Optional, Protocol from loguru import logger from rich.console import Console from basic_memory.cli.commands.cloud.rclone_installer import is_rclone_installed +from basic_memory.cli.commands.cloud.transfer import ( + ConflictStrategy, + TransferDirection, + TransferPlan, + conflict_copy_name, + strategy_overwrites_dest, +) from basic_memory.config import resolve_data_dir from basic_memory.utils import normalize_project_path @@ -225,33 +232,13 @@ def get_project_remote(project: SyncProject, bucket_name: str) -> str: # --- Directional transfer primitives (push / pull) --- # -# These power the Team-safe `bm cloud push` / `bm cloud pull` commands. Unlike -# the mirror operations (`sync`/`bisync`), they use `rclone copy` so they never -# delete on the destination, and conflicts are surfaced to the caller rather -# than silently resolved. See issue #858 for the full design rationale. - -# push = local -> cloud, pull = cloud -> local. -TransferDirection = Literal["push", "pull"] - -# How a directional transfer treats files that differ on both sides. "fail" is -# the safe default: the caller is expected to abort before any transfer runs. -ConflictStrategy = Literal["fail", "keep-local", "keep-cloud", "keep-both"] - - -@dataclass -class TransferPlan: - """Classification of how local and cloud differ for a directional transfer. - - Built from ``rclone check --combined``. Paths are relative to the project - root. ``conflicts`` are files present on both sides with differing content — - without a sync baseline (see #862) every divergence is a conflict, because - we cannot tell a teammate's edit from a stale local copy. - """ - - new: list[str] = field(default_factory=list) # only on source → safe to bring over - conflicts: list[str] = field(default_factory=list) # differ on both sides - dest_only: list[str] = field(default_factory=list) # only on destination → left untouched - errors: list[str] = field(default_factory=list) # rclone could not read/hash +# These power the `bm cloud push` / `bm cloud pull` commands on Personal +# workspaces. Unlike the mirror operations (`sync`/`bisync`), they use +# `rclone copy` so they never delete on the destination, and conflicts are +# surfaced to the caller rather than silently resolved. See issue #858 for the +# full design rationale. Team workspaces run the same commands over the WebDAV +# transport instead (`webdav_transfer.py`, #1262); the plan vocabulary both +# transports share lives in `transfer.py`. def _transfer_endpoints(project: SyncProject, bucket_name: str) -> tuple[str, str]: @@ -507,12 +494,6 @@ def project_copy( return result.returncode == 0 -def _conflict_copy_name(rel_path: str, suffix: str) -> str: - """Insert a ``.conflict-`` marker before the extension of a rel path.""" - p = PurePosixPath(rel_path) - return str(p.with_name(f"{p.stem}.conflict-{suffix}{p.suffix}")) - - def project_copy_file( project: SyncProject, bucket_name: str, @@ -560,19 +541,6 @@ def project_copy_file( return result.returncode == 0 -def _strategy_overwrites_dest(direction: TransferDirection, strategy: ConflictStrategy) -> bool: - """True when the strategy lets the source side overwrite the destination. - - The source side is cloud on pull, local on push. "keep-cloud" wins on pull, - "keep-local" wins on push; otherwise the destination is preserved. - """ - if strategy == "keep-cloud": - return direction == "pull" - if strategy == "keep-local": - return direction == "push" - return False # "fail" (no conflicts) and "keep-both" never overwrite existing dest files - - def project_transfer( project: SyncProject, bucket_name: str, @@ -597,7 +565,7 @@ def project_transfer( # beside it as a conflict copy, then do an additive (new-only) pass. if strategy == "keep-both": for rel_path in plan.conflicts: - dest_rel = _conflict_copy_name(rel_path, conflict_suffix) + dest_rel = conflict_copy_name(rel_path, conflict_suffix) copied = project_copy_file( project, bucket_name, @@ -612,7 +580,7 @@ def project_transfer( if not copied: return False - overwrite = _strategy_overwrites_dest(direction, strategy) + overwrite = strategy_overwrites_dest(direction, strategy) return project_copy( project, bucket_name, diff --git a/src/basic_memory/cli/commands/cloud/transfer.py b/src/basic_memory/cli/commands/cloud/transfer.py new file mode 100644 index 000000000..adfa38e64 --- /dev/null +++ b/src/basic_memory/cli/commands/cloud/transfer.py @@ -0,0 +1,58 @@ +"""Transport-agnostic vocabulary for directional transfers (push / pull). + +`bm cloud push` / `bm cloud pull` present one contract to the user — additive +transfers that never delete on the destination, with files that differ on both +sides surfaced rather than silently resolved — but they reach the cloud over two +different transports: rclone against object storage on Personal workspaces, and +the service's WebDAV surface on Team workspaces (see #1262). + +These types are that shared contract. They live here so neither transport has to +import the other: the WebDAV engine needs the plan vocabulary without dragging in +rclone's subprocess machinery, and vice versa. +""" + +from dataclasses import dataclass, field +from pathlib import PurePosixPath +from typing import Literal + +# push = local -> cloud, pull = cloud -> local. +TransferDirection = Literal["push", "pull"] + +# How a directional transfer treats files that differ on both sides. "fail" is +# the safe default: the caller is expected to abort before any transfer runs. +ConflictStrategy = Literal["fail", "keep-local", "keep-cloud", "keep-both"] + + +@dataclass +class TransferPlan: + """Classification of how local and cloud differ for a directional transfer. + + Paths are relative to the project root. ``conflicts`` are files present on + both sides with differing content — without a sync baseline (see #862) every + divergence is a conflict, because we cannot tell a teammate's edit from a + stale local copy. + """ + + new: list[str] = field(default_factory=list) # only on source → safe to bring over + conflicts: list[str] = field(default_factory=list) # differ on both sides + dest_only: list[str] = field(default_factory=list) # only on destination → left untouched + errors: list[str] = field(default_factory=list) # could not be compared at all + + +def conflict_copy_name(rel_path: str, suffix: str) -> str: + """Insert a ``.conflict-`` marker before the extension of a rel path.""" + p = PurePosixPath(rel_path) + return str(p.with_name(f"{p.stem}.conflict-{suffix}{p.suffix}")) + + +def strategy_overwrites_dest(direction: TransferDirection, strategy: ConflictStrategy) -> bool: + """True when the strategy lets the source side overwrite the destination. + + The source side is cloud on pull, local on push. "keep-cloud" wins on pull, + "keep-local" wins on push; otherwise the destination is preserved. + """ + if strategy == "keep-cloud": + return direction == "pull" + if strategy == "keep-local": + return direction == "push" + return False # "fail" (no conflicts) and "keep-both" never overwrite existing dest files diff --git a/src/basic_memory/cli/commands/cloud/upload.py b/src/basic_memory/cli/commands/cloud/upload.py index 30f113877..939d015be 100644 --- a/src/basic_memory/cli/commands/cloud/upload.py +++ b/src/basic_memory/cli/commands/cloud/upload.py @@ -8,6 +8,7 @@ import aiofiles import httpx +from basic_memory.cli.commands.cloud.webdav import webdav_path from basic_memory.ignore_utils import load_gitignore_patterns, should_ignore_path from basic_memory.mcp.async_client import get_client @@ -116,8 +117,9 @@ async def upload_path( skipped_count += 1 continue - # Build remote path: /webdav/{project_name}/{relative_path} - remote_path = f"/webdav/{project_name}/{relative_path}" + # Shared with the push/pull transport so both write paths + # address a project's files the same way. + remote_path = webdav_path(project_name, relative_path) print(f"Uploading {relative_path} ({i}/{len(files_to_upload)})") # Get file modification time diff --git a/src/basic_memory/cli/commands/cloud/webdav.py b/src/basic_memory/cli/commands/cloud/webdav.py new file mode 100644 index 000000000..e6642107f --- /dev/null +++ b/src/basic_memory/cli/commands/cloud/webdav.py @@ -0,0 +1,388 @@ +"""Client for the cloud WebDAV file surface. + +`bm cloud upload` already speaks the write half of this protocol: a plain +``PUT /webdav/{project}/{path}`` carrying an ``X-OC-Mtime`` header. Team +`push`/`pull` need the read half as well (#1262) — ``PROPFIND`` to enumerate a +project and ``GET`` to fetch a file — together with the validators (entity tag, +last-modified) that let a transfer decide whether two sides actually differ. + +This module sits beside ``upload.py`` rather than inside it. ``upload.py`` is the +implementation of one command: a directory walk that prints its own progress and +owns that command's filtering rules. What follows is protocol only — no CLI +output, no policy — so that the push/pull engine in ``webdav_transfer.py`` and +the upload command can share one definition of how a project's files are +addressed. + +Access control is the reason this transport exists at all. Every request here is +authorized by the service against the caller's access to *this* project, whereas +object-storage credentials are scoped to an entire tenant bucket and cannot +express per-project access. +""" + +import re +import xml.etree.ElementTree as ElementTree +from dataclasses import dataclass +from datetime import datetime +from email.utils import parsedate_to_datetime +from urllib.parse import quote, unquote, urlsplit + +import httpx + +WEBDAV_ROOT = "/webdav" + +DAV_NS = "{DAV:}" + +# The standard PROPFIND body. The properties we need (entity tag, last-modified, +# content length, resource type, display name) are all live DAV properties, so +# `allprop` asks for exactly the right set without enumerating them. +_PROPFIND_BODY = ( + '\n' +) + +# An object store reports a single-part object's entity tag as the MD5 digest of +# its bytes: 32 hex characters. Anything else — an opaque tag, a weak validator, +# or a multipart digest-of-digests with its "-N" part-count suffix — is not a +# content hash and must never be compared as one. +_CONTENT_HASH_PATTERN = re.compile(r"[0-9a-fA-F]{32}") + + +class WebdavError(Exception): + """Raised when the cloud WebDAV surface cannot be read or written.""" + + +@dataclass(frozen=True) +class RemoteFile: + """One file as the cloud reports it in a PROPFIND listing. + + ``etag`` and ``modified`` are optional because a server is free to omit + either. Callers must decide what to do without them rather than assume. + """ + + path: str # project-relative POSIX path + size: int + etag: str | None + modified: datetime | None + + +@dataclass(frozen=True) +class DownloadedFile: + """A file fetched over ``GET``, with whatever validators came back with it.""" + + content: bytes + modified: datetime | None + + +@dataclass(frozen=True) +class _Entry: + """One ```` element, before collections and files are separated.""" + + rel_path: str + is_collection: bool + size: int + etag: str | None + modified: datetime | None + + +def webdav_path(project: str, rel_path: str = "") -> str: + """Build the request path for a project, or for a file inside it. + + Callers pass the name and path unescaped; this percent-encodes them, keeping + ``/`` as the separator. Leaving that to the HTTP client is not enough: ``?`` + and ``#`` are structural URL delimiters, not path data, so a note named + ``a#draft.md`` would be requested as ``a`` — a 404, or worse, another + object's bytes. Both are legal POSIX filenames. + """ + encoded_project = quote(project, safe="") + if not rel_path: + return f"{WEBDAV_ROOT}/{encoded_project}" + return f"{WEBDAV_ROOT}/{encoded_project}/{quote(rel_path, safe='/')}" + + +def normalize_etag(raw: str | None) -> str | None: + """Strip the surrounding quotes from an entity tag, keeping any weak marker. + + The ``W/`` prefix is deliberately preserved: a weak validator promises only + semantic equivalence, never byte equality, so ``etag_content_hash`` has to be + able to see it and refuse. + """ + if raw is None: + return None + value = raw.strip() + if value.startswith("W/"): + inner = value[2:].strip().strip('"') + return f"W/{inner}" + return value.strip('"') or None + + +def etag_content_hash(etag: str | None) -> str | None: + """Return the entity tag as a usable content hash, or None when it is not one. + + "Not one" covers a missing tag, an opaque or weak tag, and the multipart + ``-`` shape — for a multipart upload the store hashes the part + digests, so the same bytes stored differently produce a different value. + Callers must fall back to another comparison rather than treating an + unusable tag as either a match or a conflict (#1262). + """ + if etag is None: + return None + if not _CONTENT_HASH_PATTERN.fullmatch(etag): + return None + return etag.lower() + + +async def list_project_files(client: httpx.AsyncClient, project: str) -> list[RemoteFile]: + """Enumerate every file in a cloud project. + + Constraint: PROPFIND answers for one collection at a time (a ``Depth: 1`` + listing), so a whole-project listing is a walk. Subdirectories are visited + breadth-first, and each path is listed only once, so a response that repeats + a directory already walked cannot send the walk round in circles. + + Raises: + WebdavError: If the service rejects a listing or returns XML we cannot + interpret. + """ + files: list[RemoteFile] = [] + pending = [""] # project-relative directories; "" is the project root + visited: set[str] = set() + + while pending: + rel_dir = pending.pop(0) + if rel_dir in visited: + continue + visited.add(rel_dir) + + for entry in await _propfind(client, project, rel_dir): + if entry.is_collection: + pending.append(entry.rel_path) + else: + files.append( + RemoteFile( + path=entry.rel_path, + size=entry.size, + etag=entry.etag, + modified=entry.modified, + ) + ) + + return files + + +async def download_file(client: httpx.AsyncClient, project: str, rel_path: str) -> DownloadedFile: + """Fetch one file, along with the last-modified time the service reports. + + Raises: + WebdavError: If the service refuses the download. + """ + request_path = webdav_path(project, rel_path) + try: + response = await client.get(request_path) + response.raise_for_status() + except httpx.HTTPError as exc: + raise WebdavError(f"Failed to download {rel_path}: {_describe(exc)}") from exc + + return DownloadedFile( + content=response.content, + modified=_parse_http_date(response.headers.get("Last-Modified")), + ) + + +async def upload_file( + client: httpx.AsyncClient, + project: str, + rel_path: str, + *, + content: bytes, + mtime: int, + create_only: bool = False, +) -> bool: + """Write one file, advertising the local modification time. + + ``X-OC-Mtime`` (the ownCloud/Nextcloud convention) is what `bm cloud upload` + already sends, so the two write paths look identical to the service. + + ``create_only`` sends ``If-None-Match: *``, which asks the service to refuse + the write if the resource already exists. That precondition is evaluated at + the moment of the write, which is the only place a client-side check cannot + reach: any listing this client did beforehand is already stale by the time + the request lands. + + Returns: + True when the file was written; False when a create-only write was + refused because the resource already exists. + + Raises: + WebdavError: If the service refuses the upload for any other reason. + """ + request_path = webdav_path(project, rel_path) + headers = {"X-OC-Mtime": str(mtime)} + if create_only: + headers["If-None-Match"] = "*" + + try: + response = await client.put(request_path, content=content, headers=headers) + # Checked before raise_for_status: a refused precondition is the answer + # this call asked for, not a failure. + if create_only and response.status_code == httpx.codes.PRECONDITION_FAILED: + return False + response.raise_for_status() + except httpx.HTTPError as exc: + raise WebdavError(f"Failed to upload {rel_path}: {_describe(exc)}") from exc + + return True + + +# --- PROPFIND parsing --- + + +async def _propfind(client: httpx.AsyncClient, project: str, rel_dir: str) -> list[_Entry]: + """List one collection, returning its immediate children.""" + request_path = webdav_path(project, rel_dir) + try: + response = await client.request( + "PROPFIND", + request_path, + content=_PROPFIND_BODY, + headers={"Depth": "1", "Content-Type": "application/xml"}, + ) + response.raise_for_status() + except httpx.HTTPError as exc: + raise WebdavError(f"Failed to list cloud project '{project}': {_describe(exc)}") from exc + + return _parse_propfind(response.text, request_path=request_path, rel_dir=rel_dir) + + +def _parse_propfind(xml_text: str, *, request_path: str, rel_dir: str) -> list[_Entry]: + """Turn a multistatus document into this collection's immediate children. + + The document is served by the authenticated cloud service, not by arbitrary + third parties, so it is parsed with the standard library parser. + """ + try: + root = ElementTree.fromstring(xml_text) + except ElementTree.ParseError as exc: + raise WebdavError(f"Could not parse the cloud listing for '{request_path}': {exc}") from exc + + entries: list[_Entry] = [] + for index, response in enumerate(root.findall(f"{DAV_NS}response")): + href = _text(response.find(f"{DAV_NS}href")) + + # Trigger: the first response element describes the collection we asked + # for (RFC 4918 includes the resource itself in a Depth: 1 listing). + # Why: only the first is checked — a subdirectory whose href happens to + # collide with the request path is still a real child, and dropping it + # would silently hide every file beneath it. + # Outcome: skip the self entry, keep everything else. + if index == 0 and href is not None and _same_path(href, request_path): + continue + + props = _merged_props(response) + name = _entry_name(props, href) + if name is None: + raise WebdavError( + f"The cloud listing for '{request_path}' contains an entry with no name" + ) + + entries.append( + _Entry( + rel_path=f"{rel_dir}/{name}" if rel_dir else name, + is_collection=_is_collection(props), + size=_parse_size(props), + etag=normalize_etag(_text(props.get("getetag"))), + modified=_parse_http_date(_text(props.get("getlastmodified"))), + ) + ) + + return entries + + +def _merged_props(response: ElementTree.Element) -> dict[str, ElementTree.Element]: + """Collect the properties of one response element, keyed by local tag name. + + Propstat blocks are merged without inspecting their status: a non-2xx block + carries empty property elements, which read as "absent" anyway, so filtering + on status would only add a branch that changes nothing. + """ + props: dict[str, ElementTree.Element] = {} + for propstat in response.findall(f"{DAV_NS}propstat"): + prop = propstat.find(f"{DAV_NS}prop") + if prop is None: + continue + for child in prop: + props.setdefault(child.tag.removeprefix(DAV_NS), child) + return props + + +def _entry_name(props: dict[str, ElementTree.Element], href: str | None) -> str | None: + """Resolve an entry's basename. + + ``displayname`` is preferred because it is the literal name, free of any URL + encoding. The href's last segment is the fallback for servers that omit it. + """ + display_name = _text(props.get("displayname")) + if display_name: + return display_name + if href is None: + return None + segments = [segment for segment in _href_path(href).split("/") if segment] + return segments[-1] if segments else None + + +def _is_collection(props: dict[str, ElementTree.Element]) -> bool: + resource_type = props.get("resourcetype") + if resource_type is None: + return False + return resource_type.find(f"{DAV_NS}collection") is not None + + +def _parse_size(props: dict[str, ElementTree.Element]) -> int: + """Read getcontentlength, treating an absent or empty value as zero bytes.""" + raw = _text(props.get("getcontentlength")) + if not raw: + return 0 + try: + return int(raw) + except ValueError as exc: + raise WebdavError(f"The cloud reported a non-numeric file size: {raw!r}") from exc + + +def _parse_http_date(raw: str | None) -> datetime | None: + """Parse an RFC 1123 HTTP-date into an aware datetime, or None when absent.""" + if not raw: + return None + try: + return parsedate_to_datetime(raw) + except (TypeError, ValueError) as exc: + raise WebdavError(f"The cloud reported an unparseable timestamp: {raw!r}") from exc + + +def _text(element: ElementTree.Element | None) -> str | None: + if element is None or element.text is None: + return None + return element.text.strip() + + +def _href_path(href: str) -> str: + """Return the decoded path component of an href. + + RFC 4918 hrefs are URIs, so percent-encoding is decoded here before the path + is compared or split into segments. + """ + return unquote(urlsplit(href).path) + + +def _same_path(href: str, request_path: str) -> bool: + """Compare an href against a request path, ignoring a trailing slash. + + Both sides are percent-decoded first: the request path is encoded by + ``webdav_path`` while the href may or may not be, and the comparison is + about which resource is named, not how it was spelled on the wire. + """ + return _href_path(href).rstrip("/") == unquote(request_path).rstrip("/") + + +def _describe(exc: httpx.HTTPError) -> str: + """Render an httpx failure as a single actionable line.""" + if isinstance(exc, httpx.HTTPStatusError): + return f"HTTP {exc.response.status_code} - {exc.response.text.strip()}" + return str(exc) diff --git a/src/basic_memory/cli/commands/cloud/webdav_transfer.py b/src/basic_memory/cli/commands/cloud/webdav_transfer.py new file mode 100644 index 000000000..0c824f1c3 --- /dev/null +++ b/src/basic_memory/cli/commands/cloud/webdav_transfer.py @@ -0,0 +1,590 @@ +"""Directional transfers (`bm cloud push` / `bm cloud pull`) over WebDAV. + +On Personal workspaces these transfers run through rclone against object +storage. That requires tenant-scoped storage credentials, which are scoped to a +whole bucket and therefore cannot express "this member may read project A but +not project B" — on a Team workspace they would grant more access than the +service itself does, and minting them is restricted to workspace owners anyway, +so members were simply stuck (#1262). + +The WebDAV surface enforces access where it belongs: every request is authorized +against the caller's access to the specific project. This module is the same +transfer engine over that transport, and it keeps the same contract: + +- additive — nothing is ever deleted on the destination +- files that differ on both sides are conflicts, resolved only by an explicit + ``--on-conflict`` choice +- a path the plan cleared as new is only ever created, never used to replace + something that arrived in the meantime — enforced at the write itself, by an + exclusive create on pull and a conditional create on push +- deletions are not propagated (see #862) + +Comparison is by entity tag plus size, falling back to last-modified plus size +when the service reports no entity tag we can treat as a content hash. See +``_compare`` for why that fallback errs toward reporting a conflict, and +``_drop_appeared_on_cloud`` for how a stale plan is kept from overwriting a note +nobody compared. +""" + +import hashlib +import os +import tempfile +from collections.abc import Callable +from contextlib import AbstractAsyncContextManager +from dataclasses import dataclass +from functools import partial +from pathlib import Path, PurePosixPath +from typing import Literal + +import httpx +from rich.console import Console + +from basic_memory.cli.commands.cloud.transfer import ( + ConflictStrategy, + TransferDirection, + TransferPlan, + conflict_copy_name, + strategy_overwrites_dest, +) +from basic_memory.cli.commands.cloud.webdav import ( + DownloadedFile, + RemoteFile, + WebdavError, + download_file, + etag_content_hash, + list_project_files, + upload_file, +) +from basic_memory.ignore_utils import load_gitignore_patterns, should_ignore_path +from basic_memory.mcp.async_client import get_cloud_proxy_client + +console = Console() + +ClientFactory = Callable[[], AbstractAsyncContextManager[httpx.AsyncClient]] + +# How far two timestamps may drift and still be considered the same instant. +# HTTP-date has one-second resolution, so a local mtime of 10.9s and a reported +# time of 10s describe the same write. Kept tight on purpose: widening this +# window trades a rare spurious conflict for the risk of calling two different +# files identical, and losing an edit is the worse outcome. +MODIFY_WINDOW_SECONDS = 1.0 + +_HASH_CHUNK_BYTES = 1024 * 1024 + +# Whether two copies of a path hold the same bytes. "unknown" means the question +# could not be answered at all — never a silent "same". +Comparison = Literal["same", "differ", "unknown"] + + +@dataclass(frozen=True) +class LocalFile: + """One file on this machine, addressed the same way the cloud addresses it.""" + + path: str # project-relative POSIX path + size: int + mtime: float + + +@dataclass(frozen=True) +class _Transfer: + """One file to move, and whether it may replace something already there. + + ``create_only`` carries the plan's classification forward: a path the diff + called `new` was cleared to be created, never to overwrite. That distinction + is what makes the destination re-check below safe to act on. + """ + + source_rel: str + dest_rel: str + create_only: bool + + def describe(self) -> str: + if self.source_rel == self.dest_rel: + return self.source_rel + return f"{self.source_rel} -> {self.dest_rel}" + + +# --- Entry points used by the CLI --- + + +async def webdav_project_diff( + project: str, + local_root: Path, + direction: TransferDirection, + *, + workspace_id: str, + client_cm_factory: ClientFactory | None = None, +) -> TransferPlan: + """Classify how local and cloud differ, without transferring anything. + + Mirrors ``project_diff`` on the rclone path: the caller inspects the plan and + decides whether to abort before any bytes move. + + Raises: + WebdavError: If the project cannot be listed. + """ + cm_factory = client_cm_factory or partial(get_cloud_proxy_client, workspace=workspace_id) + async with cm_factory() as client: + remote_files = await list_project_files(client, project) + + return build_transfer_plan( + local_root=local_root, + remote_files=remote_files, + direction=direction, + ) + + +async def webdav_project_transfer( + project: str, + local_root: Path, + direction: TransferDirection, + plan: TransferPlan, + *, + workspace_id: str, + strategy: ConflictStrategy = "fail", + conflict_suffix: str = "", + dry_run: bool = False, + verbose: bool = False, + client_cm_factory: ClientFactory | None = None, +) -> None: + """Execute a directional transfer for the chosen conflict strategy. + + Callers detect conflicts with ``webdav_project_diff`` first and abort when + ``strategy == "fail"`` and conflicts exist; this function assumes that gate + has already passed and applies the resolution. + + Raises: + WebdavError: If any transfer fails, or if the cloud names a file that + would be written outside the project directory. + """ + # keep-both: preserve the destination's version and drop the incoming one + # beside it as a conflict copy, then do an additive (new-only) pass. + renames = ( + [ + _Transfer(rel_path, conflict_copy_name(rel_path, conflict_suffix), create_only=True) + for rel_path in plan.conflicts + ] + if strategy == "keep-both" + else [] + ) + + # A path the plan called `new` must only ever be created. A path the user + # resolved with keep-local/keep-cloud is an instruction to overwrite. + overwrite = strategy_overwrites_dest(direction, strategy) + copies = [_Transfer(rel_path, rel_path, create_only=True) for rel_path in plan.new] + if overwrite: + copies.extend( + _Transfer(rel_path, rel_path, create_only=False) for rel_path in plan.conflicts + ) + + transfers = renames + copies + if not transfers: + console.print("[dim]Nothing to transfer.[/dim]") + return + + if dry_run: + console.print(f"[dim]Dry run: {len(transfers)} file(s) would be transferred.[/dim]") + for transfer in transfers: + console.print(f" [dim]{transfer.describe()}[/dim]") + return + + cm_factory = client_cm_factory or partial(get_cloud_proxy_client, workspace=workspace_id) + async with cm_factory() as client: + if direction == "push": + transfers, appeared = await _drop_appeared_on_cloud(client, project, transfers) + else: + appeared = [] + + transferred = 0 + for transfer in transfers: + if verbose: + console.print(f" {transfer.describe()}") + if direction == "pull": + written = await _pull_file(client, project, local_root, transfer) + else: + written = await _push_file(client, project, local_root, transfer) + if not written: + appeared.append(transfer.dest_rel) + continue + transferred += 1 + + console.print(f"[dim]Transferred {transferred} file(s).[/dim]") + _report_appeared(appeared) + + +# --- Planning --- + + +def build_transfer_plan( + *, + local_root: Path, + remote_files: list[RemoteFile], + direction: TransferDirection, +) -> TransferPlan: + """Compare both sides and classify every path into the transfer plan. + + The ignore patterns are applied to the cloud listing as well as the local + scan, so an ignored path is invisible on both sides — the same thing rclone's + ``--filter-from`` does for the Personal path. + """ + ignore_patterns = load_gitignore_patterns(local_root, use_gitignore=False) + local_files = scan_local_files(local_root, ignore_patterns) + + remote_by_path: dict[str, RemoteFile] = {} + for remote in remote_files: + # Validate here rather than at write time: a listing that names a path + # outside the project is a broken or hostile response, and the user + # should see that before a plan is presented, not mid-transfer. + local_equivalent = _safe_local_path(local_root, remote.path) + if should_ignore_path(local_equivalent, local_root, ignore_patterns): + continue + remote_by_path[remote.path] = remote + + source_paths, dest_paths = ( + (set(remote_by_path), set(local_files)) + if direction == "pull" + else (set(local_files), set(remote_by_path)) + ) + + plan = TransferPlan( + new=sorted(source_paths - dest_paths), + dest_only=sorted(dest_paths - source_paths), + ) + + for path in sorted(source_paths & dest_paths): + comparison = _compare(local_files[path], remote_by_path[path], local_root) + if comparison == "differ": + plan.conflicts.append(path) + elif comparison == "unknown": + plan.errors.append(path) + + return plan + + +def scan_local_files(local_root: Path, ignore_patterns: set[str]) -> dict[str, LocalFile]: + """Walk the project directory, skipping anything the ignore patterns match. + + Only ``.bmignore`` patterns apply, matching the filter the rclone path builds + for push/pull. A project's ``.gitignore`` deliberately does not participate: + it is scoped to `bm cloud upload`, and honoring it here would make a transfer + depend on which machine ran it. + + Links are not followed and symlinked files are skipped, so push can never + read bytes from outside the project boundary — the same rule the local + project scanner applies for the same reason. + """ + files: dict[str, LocalFile] = {} + + for root, dirs, filenames in os.walk(local_root, followlinks=False): + root_path = Path(root) + dirs[:] = [ + name + for name in dirs + if not (root_path / name).is_symlink() + and not should_ignore_path(root_path / name, local_root, ignore_patterns) + ] + + for filename in filenames: + file_path = root_path / filename + if file_path.is_symlink(): + continue + if should_ignore_path(file_path, local_root, ignore_patterns): + continue + stat = file_path.stat() + rel_path = file_path.relative_to(local_root).as_posix() + files[rel_path] = LocalFile(path=rel_path, size=stat.st_size, mtime=stat.st_mtime) + + return files + + +def _compare(local: LocalFile, remote: RemoteFile, local_root: Path) -> Comparison: + """Decide whether two copies of a path hold the same bytes. + + Size settles it whenever it differs, and is checked first so a large file is + never read just to learn what its length already proved. + + When the entity tag is a content hash, the comparison is exact. When it is + not — absent, opaque, or the multipart ``-N`` shape — the fallback is + last-modified plus size, and matching timestamps are required for a "same" + verdict. That errs toward reporting a conflict, which the user can resolve + with an explicit ``--on-conflict`` choice; the opposite error would silently + skip a file that really did change and lose an edit. + + The fallback is deliberately the weaker path. A pull carries the cloud's + timestamp onto the local copy, so the two line up afterwards; a push cannot, + because the stored timestamp is when the write landed rather than when the + file was edited. A file that lacks a usable entity tag and was last pushed + from here will therefore keep reporting as a conflict until the tag is + comparable again — visible and recoverable, unlike a lost edit. + """ + if local.size != remote.size: + return "differ" + + content_hash = etag_content_hash(remote.etag) + if content_hash is not None: + return "same" if _file_content_hash(local_root / local.path) == content_hash else "differ" + + if remote.modified is None: + return "unknown" + + drift = abs(remote.modified.timestamp() - local.mtime) + return "same" if drift <= MODIFY_WINDOW_SECONDS else "differ" + + +def _file_content_hash(path: Path) -> str: + """Hash a local file for comparison against the store's entity tag. + + MD5 is not a choice here — it is the digest the object store reports for a + single-part object. This is a content fingerprint, never a security control. + """ + digest = hashlib.md5(usedforsecurity=False) + with path.open("rb") as handle: + for chunk in iter(lambda: handle.read(_HASH_CHUNK_BYTES), b""): + digest.update(chunk) + return digest.hexdigest() + + +# --- Guarding against a destination that moved under the plan --- +# +# The plan is a snapshot. Between classifying a path as `new` and writing it, the +# destination can gain that path — a teammate's push, another machine's pull, the +# user's own editor. Acting on the stale classification would destroy a note that +# nobody asked to replace, even under the default `--on-conflict fail`. +# +# The Personal path does not have this problem, and not because of its plan: +# `project_copy` passes `--ignore-existing`, and rclone evaluates that against +# the destination listing it makes at copy time, not against the earlier +# `rclone check`. So a file that appeared in between is skipped there. +# +# What actually holds the line here is a precondition evaluated at the write +# itself, per direction: an exclusive create on pull, and `If-None-Match: *` on +# push. Both are atomic with the write, so no listing this client made — however +# recent — is standing between a teammate and their note. The re-list below is +# an optimization and a reporting aid, not the correctness boundary: it spares +# pointless round trips and names what was skipped, and it is allowed to be +# stale, because the write refuses on its own. + + +async def _drop_appeared_on_cloud( + client: httpx.AsyncClient, project: str, transfers: list[_Transfer] +) -> tuple[list[_Transfer], list[str]]: + """Re-read the cloud and drop create-only pushes whose path now exists. + + The analogue of rclone re-listing the destination at copy time: it avoids + uploading bytes that are certain to be refused, and it names the collisions + up front instead of one at a time. It is deliberately not the safety + mechanism — the window between this listing and the Nth PUT grows with every + file ahead of it in the queue. ``If-None-Match: *`` on each create-only + upload is what closes that window. + """ + create_only = {transfer.dest_rel for transfer in transfers if transfer.create_only} + if not create_only: + return transfers, [] + + existing = {remote.path for remote in await list_project_files(client, project)} + appeared = sorted(create_only & existing) + if not appeared: + return transfers, [] + + kept = [ + transfer + for transfer in transfers + if not (transfer.create_only and transfer.dest_rel in existing) + ] + return kept, appeared + + +def _report_appeared(appeared: list[str]) -> None: + """Name the files that were left alone because the destination gained them.""" + if not appeared: + return + + console.print( + f"[yellow]{len(appeared)} file(s) appeared on the destination after this transfer " + "was planned and were left untouched:[/yellow]" + ) + for path in appeared: + console.print(f" [yellow]*[/yellow] {path}") + console.print("[dim]Re-run to compare them and resolve with --on-conflict.[/dim]") + + +# --- Single-file transfers --- + + +async def _pull_file( + client: httpx.AsyncClient, + project: str, + local_root: Path, + transfer: _Transfer, +) -> bool: + """Download one cloud file and land it under the destination path. + + Nothing is created at the destination until the bytes exist: the download + lands in a sibling temp file, and only then is the name claimed. Returns + False when a create-only transfer found the name already taken, so the + caller can report it instead of replacing a note it never compared. + """ + target = _safe_local_path(local_root, transfer.dest_rel) + if not transfer.create_only: + _refuse_symlink(target, transfer.dest_rel) + + downloaded = await download_file(client, project, transfer.source_rel) + + target.parent.mkdir(parents=True, exist_ok=True) + temp_path = _write_temp_file(target, downloaded) + try: + if transfer.create_only: + return _publish_new(temp_path, target, downloaded) + # An explicit keep-cloud is an instruction to replace what is there. + os.replace(temp_path, target) + return True + finally: + # After a rename the temp name is already gone; after a link or a + # direct write it is the copy to drop. + temp_path.unlink(missing_ok=True) + + +def _write_temp_file(target: Path, downloaded: DownloadedFile) -> Path: + """Stage the downloaded bytes beside the destination, fully written.""" + handle, temp_name = tempfile.mkstemp( + dir=target.parent, prefix=f".{target.name}.", suffix=".part" + ) + temp_path = Path(temp_name) + try: + with os.fdopen(handle, "wb") as stream: + stream.write(downloaded.content) + # The timestamp is set here, before publication, because it lives on the + # inode — a hardlinked publish shares it, and there is no window in which + # the published note carries the wrong mtime. + _apply_modified(temp_path, downloaded) + except BaseException: + temp_path.unlink(missing_ok=True) + raise + + return temp_path + + +def _publish_new(temp_path: Path, target: Path, downloaded: DownloadedFile) -> bool: + """Claim a name that nothing else holds, atomically. + + ``os.link`` is the atomic no-replace publish: it fails outright when the + name is taken, so a note that appeared during the download is never + destroyed, and no reader ever observes a half-written file. + """ + try: + os.link(temp_path, target) + return True + except FileExistsError: + return False + except OSError: + # Trigger: the filesystem cannot hardlink at all — exFAT, and the + # virtual/network mounts this project already accommodates elsewhere. + # Why: refusing outright would break pull for those users, and the + # property that has to hold — never destroying a note that appeared — + # does not actually need links. An exclusive create claims the name just + # as atomically. + # Outcome: the same no-clobber guarantee, weaker only in that a reader + # can catch the new file mid-write. No unlinked filesystem can do + # better, and a real failure (no space, no permission) still surfaces + # from the create below rather than being swallowed here. + return _publish_new_without_link(target, downloaded) + + +def _publish_new_without_link(target: Path, downloaded: DownloadedFile) -> bool: + """Claim the name with an exclusive create, then write through it.""" + try: + handle = os.open(target, os.O_CREAT | os.O_EXCL | os.O_WRONLY) + except FileExistsError: + return False + + try: + with os.fdopen(handle, "wb") as stream: + stream.write(downloaded.content) + except BaseException: + # Never leave a note behind that holds none of the content. + target.unlink(missing_ok=True) + raise + + _apply_modified(target, downloaded) + return True + + +def _apply_modified(path: Path, downloaded: DownloadedFile) -> None: + """Carry the cloud's timestamp onto the local copy, the way rclone does. + + Without it every pulled file would look freshly modified, and the + last-modified fallback in ``_compare`` could never report a match. + """ + if downloaded.modified is None: + return + stamp = downloaded.modified.timestamp() + os.utime(path, (stamp, stamp)) + + +async def _push_file( + client: httpx.AsyncClient, + project: str, + local_root: Path, + transfer: _Transfer, +) -> bool: + """Upload one local file to the destination path in the cloud project. + + Returns False when a create-only upload was refused because the path now + exists in the cloud, mirroring the pull side. + """ + source = _safe_local_path(local_root, transfer.source_rel) + _refuse_symlink(source, transfer.source_rel) + stat = source.stat() + return await upload_file( + client, + project, + transfer.dest_rel, + content=source.read_bytes(), + mtime=int(stat.st_mtime), + create_only=transfer.create_only, + ) + + +def _safe_local_path(local_root: Path, rel_path: str) -> Path: + """Resolve a project-relative path, refusing anything that escapes the project. + + On pull these paths come from the service's listing, so remote input decides + where this machine writes. An absolute path, a ``..`` segment, or a Windows + separator must never be honored. + + Those are lexical checks, and a lexical check cannot see a link. The parent + chain — the part that actually decides which directory is read from or + written into — is resolved and required to stay inside the project. + Otherwise a symlinked directory would let push read bytes from outside the + project into a shared workspace, and let pull write through to somewhere the + user never pointed at. + + The final component is left to the caller: whether a link there should be + refused or simply treated as "already taken" depends on what the transfer is + about to do to it. + + Raises: + WebdavError: If the path would resolve outside the project directory. + """ + candidate = PurePosixPath(rel_path) + if "\\" in rel_path or candidate.is_absolute() or ".." in candidate.parts: + raise WebdavError(f"Refusing to transfer a path outside the project: {rel_path!r}") + + target = local_root.joinpath(*candidate.parts) + root = Path(os.path.realpath(local_root)) + parent = Path(os.path.realpath(target.parent)) + if parent != root and root not in parent.parents: + raise WebdavError(f"Refusing to transfer through a link out of the project: {rel_path!r}") + + return target + + +def _refuse_symlink(path: Path, rel_path: str) -> None: + """Refuse to read from, or write over, a path that is itself a link. + + Used where the transfer would otherwise follow it: reading a push source, or + replacing a file the user resolved with keep-cloud. Create-only pulls need no + such check — their exclusive create already refuses a link outright. + """ + if path.is_symlink(): + raise WebdavError(f"Refusing to transfer a symlinked path: {rel_path!r}") diff --git a/tests/cli/cloud/test_project_sync_command.py b/tests/cli/cloud/test_project_sync_command.py index 3055930ab..357effeea 100644 --- a/tests/cli/cloud/test_project_sync_command.py +++ b/tests/cli/cloud/test_project_sync_command.py @@ -1,6 +1,7 @@ """Tests for cloud sync and bisync command behavior.""" import importlib +import re from types import SimpleNamespace import pytest @@ -8,7 +9,9 @@ from typer.testing import CliRunner from basic_memory.cli.app import app -from basic_memory.cli.commands.cloud.rclone_commands import RcloneError, TransferPlan +from basic_memory.cli.commands.cloud.rclone_commands import RcloneError +from basic_memory.cli.commands.cloud.transfer import TransferPlan +from basic_memory.cli.commands.cloud.webdav import WebdavError from basic_memory.config import ProjectEntry, ProjectMode from basic_memory.schemas.cloud import WorkspaceInfo from typing import Any @@ -16,6 +19,15 @@ runner = CliRunner() +def _plain(text: str) -> str: + """Strip console styling so assertions read against the words alone. + + Rich emits escape sequences for styled output (``[dim]`` survives even + NO_COLOR), which would otherwise split the phrases these tests look for. + """ + return " ".join(re.sub(r"\x1b\[[0-9;]*m", "", text).split()) + + @pytest.mark.parametrize( "command", ["sync", "bisync", "check", "bisync-reset", "prune"], @@ -566,54 +578,39 @@ def test_cloud_push_keep_local_resolves_conflict(monkeypatch, config_manager): assert recorder["args"][2] == "push" -def test_cloud_push_allows_organization_workspace(monkeypatch, config_manager): - """push is additive and Team-safe — an organization workspace is allowed (no Personal gate).""" - module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") - - org_ws = _workspace("team-tenant", "organization", "acme", is_default=False) - plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=[], errors=[]) - recorder: dict[str, Any] = {} - _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=org_ws) - - result = runner.invoke(app, ["cloud", "push", "--name", "research"]) - - assert result.exit_code == 0, result.output - assert "research push completed successfully" in result.output - # Routed through the team workspace's own remote, against its tenant's bucket. - assert recorder["args"][0].remote_name == "basic-memory-cloud-acme" - - def test_cloud_pull_workspace_override_routes_through_workspace_remote(monkeypatch, config_manager): """pull --workspace routes through the named workspace's own remote and bucket.""" module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") - org_ws = _workspace("team-tenant", "organization", "acme", is_default=False) + alt_ws = _workspace("personal-alt-tenant", "personal", "personal-alt", is_default=False) plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=[], errors=[]) recorder: dict[str, Any] = {} - # _get_workspace_for_project must receive the override and return the org workspace. + # _get_workspace_for_project must receive the override and return that workspace. def _resolve(_name, _config, *, workspace_override=None): - assert workspace_override == "acme" - return _async_value(org_ws) + assert workspace_override == "personal-alt" + return _async_value(alt_ws) - _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=org_ws) + _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=alt_ws) monkeypatch.setattr(module, "_get_workspace_for_project", _resolve) - result = runner.invoke(app, ["cloud", "pull", "--name", "research", "--workspace", "acme"]) + result = runner.invoke( + app, ["cloud", "pull", "--name", "research", "--workspace", "personal-alt"] + ) assert result.exit_code == 0, result.output - assert recorder["args"][0].remote_name == "basic-memory-cloud-acme" + assert recorder["args"][0].remote_name == "basic-memory-cloud-personal-alt" assert recorder["args"][2] == "pull" def test_cloud_push_errors_when_workspace_remote_not_set_up(monkeypatch, config_manager): - """If the workspace's remote isn't configured, push stops with the setup command.""" + """If a Personal workspace's remote isn't configured, push stops with the setup command.""" module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") - org_ws = _workspace("team-tenant", "organization", "acme", is_default=False) + alt_ws = _workspace("personal-alt-tenant", "personal", "personal-alt", is_default=False) plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=[], errors=[]) recorder: dict[str, Any] = {} - _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=org_ws) + _stub_transfer_env(monkeypatch, module, plan=plan, recorder=recorder, workspace=alt_ws) # Override: this workspace has not been set up yet. monkeypatch.setattr(module, "rclone_remote_exists", lambda _remote: False) @@ -622,10 +619,169 @@ def test_cloud_push_errors_when_workspace_remote_not_set_up(monkeypatch, config_ assert result.exit_code == 1, result.output output = " ".join(result.output.split()) assert "not set up for sync" in output - assert "bm cloud setup --workspace acme" in output + assert "bm cloud setup --workspace personal-alt" in output assert "args" not in recorder # never transferred +# --- Team workspaces transfer over WebDAV (#1262) --- + + +def _stub_webdav_transfer_env(monkeypatch, module, *, plan, recorder, workspace=None): + """Stub the Team push/pull chain so only the WebDAV routing is exercised.""" + monkeypatch.setattr(module, "_require_cloud_credentials", lambda _config: None) + ws = workspace or _workspace("team-tenant", "organization", "acme", is_default=False) + monkeypatch.setattr( + module, + "_get_workspace_for_project", + lambda _name, _config, **_kwargs: _async_value(ws), + ) + monkeypatch.setattr( + module, + "_get_cloud_project", + lambda _name, **_kwargs: _async_value(SimpleNamespace(name="research", path="research")), + ) + monkeypatch.setattr(module, "_require_local_sync_path", lambda _name, _config: "/tmp/research") + + # Nothing on the Team path may reach for storage credentials or an rclone + # remote — that requirement is exactly what made these commands unusable. + def _forbidden(*_args, **_kwargs): + raise AssertionError("the Team path must not touch rclone or storage credentials") + + monkeypatch.setattr(module, "get_mount_info", _forbidden) + monkeypatch.setattr(module, "rclone_remote_exists", _forbidden) + monkeypatch.setattr(module, "project_diff", _forbidden) + monkeypatch.setattr(module, "project_transfer", _forbidden) + + async def _fake_diff(*args, **kwargs): + recorder["diff_args"] = args + recorder["diff_kwargs"] = kwargs + return plan + + async def _fake_transfer(*args, **kwargs): + recorder["args"] = args + recorder["kwargs"] = kwargs + + monkeypatch.setattr(module, "webdav_project_diff", _fake_diff) + monkeypatch.setattr(module, "webdav_project_transfer", _fake_transfer) + + +@pytest.mark.parametrize("direction", ["pull", "push"]) +def test_cloud_transfer_on_team_workspace_uses_webdav(monkeypatch, config_manager, direction): + """Team push/pull run over WebDAV — no rclone remote, no `bm cloud setup`.""" + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=["local-only.md"], errors=[]) + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=plan, recorder=recorder) + + result = runner.invoke(app, ["cloud", direction, "--name", "research"]) + + assert result.exit_code == 0, result.output + output = _plain(result.output) + assert f"research {direction} completed successfully" in output + assert "bm cloud setup" not in output + assert "deletions are not propagated" in output + # Routed at the resolved workspace, addressed by the cloud project's name. + assert recorder["diff_args"][0] == "research" + assert recorder["diff_args"][2] == direction + assert recorder["diff_kwargs"]["workspace_id"] == "team-tenant" + assert recorder["kwargs"]["strategy"] == "fail" + + +def test_cloud_pull_on_team_workspace_aborts_on_conflict_by_default(monkeypatch, config_manager): + """The default conflict gate is the same on both transports.""" + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + plan = TransferPlan(new=[], conflicts=["notes/dup.md"], dest_only=[], errors=[]) + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=plan, recorder=recorder) + + result = runner.invoke(app, ["cloud", "pull", "--name", "research"]) + + assert result.exit_code == 1, result.output + output = _plain(result.output) + assert "notes/dup.md" in output + assert "--on-conflict keep-cloud" in output + assert "args" not in recorder # transfer never ran + + +@pytest.mark.parametrize("strategy", ["keep-local", "keep-cloud", "keep-both"]) +def test_cloud_push_on_team_workspace_passes_the_conflict_strategy( + monkeypatch, config_manager, strategy +): + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + plan = TransferPlan(new=[], conflicts=["notes/dup.md"], dest_only=[], errors=[]) + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=plan, recorder=recorder) + + result = runner.invoke(app, ["cloud", "push", "--name", "research", "--on-conflict", strategy]) + + assert result.exit_code == 0, result.output + assert recorder["kwargs"]["strategy"] == strategy + assert recorder["kwargs"]["conflict_suffix"] + + +def test_cloud_pull_on_team_workspace_aborts_when_files_cannot_be_compared( + monkeypatch, config_manager +): + """Without a validator to compare, pull stops rather than guessing.""" + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + plan = TransferPlan(new=[], conflicts=[], dest_only=[], errors=["opaque.md"]) + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=plan, recorder=recorder) + + result = runner.invoke(app, ["cloud", "pull", "--name", "research"]) + + assert result.exit_code == 1, result.output + output = _plain(result.output) + assert "no comparable checksum or timestamp" in output + assert "opaque.md" in output + assert "args" not in recorder + + +def test_cloud_pull_on_team_workspace_forwards_dry_run_and_verbose(monkeypatch, config_manager): + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + plan = TransferPlan(new=["new.md"], conflicts=[], dest_only=[], errors=[]) + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=plan, recorder=recorder) + + result = runner.invoke(app, ["cloud", "pull", "--name", "research", "--dry-run", "--verbose"]) + + assert result.exit_code == 0, result.output + assert recorder["kwargs"]["dry_run"] is True + assert recorder["kwargs"]["verbose"] is True + + +def test_cloud_pull_on_team_workspace_reports_a_webdav_failure(monkeypatch, config_manager): + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=TransferPlan(), recorder=recorder) + + async def _raise(*_args, **_kwargs): + raise WebdavError("HTTP 403 - Forbidden") + + monkeypatch.setattr(module, "webdav_project_diff", _raise) + + result = runner.invoke(app, ["cloud", "pull", "--name", "research"]) + + assert result.exit_code == 1, result.output + output = _plain(result.output) + assert "Pull error: HTTP 403 - Forbidden" in output + # The old failure pointed at a command the member could never run. + assert "bm cloud setup" not in output + + +def test_cloud_pull_on_team_workspace_reports_a_missing_project(monkeypatch, config_manager): + module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") + recorder: dict[str, Any] = {} + _stub_webdav_transfer_env(monkeypatch, module, plan=TransferPlan(), recorder=recorder) + monkeypatch.setattr(module, "_get_cloud_project", lambda _name, **_kwargs: _async_value(None)) + + result = runner.invoke(app, ["cloud", "pull", "--name", "research"]) + + assert result.exit_code == 1, result.output + assert "not found" in _plain(result.output) + assert "diff_args" not in recorder + + def test_get_workspace_for_project_override_resolves(monkeypatch, config_manager): """An explicit --workspace override selects that workspace regardless of config.""" module = importlib.import_module("basic_memory.cli.commands.cloud.project_sync") diff --git a/tests/cli/cloud/test_webdav_client.py b/tests/cli/cloud/test_webdav_client.py new file mode 100644 index 000000000..ae698bc83 --- /dev/null +++ b/tests/cli/cloud/test_webdav_client.py @@ -0,0 +1,470 @@ +"""Tests for the WebDAV client used by Team push/pull (#1262). + +PROPFIND responses are fixtures rather than live calls: the validators these +tests rely on (entity tag, last-modified) are part of the contract this client +codes against, and a live cloud is not the thing under test here. +""" + +from contextlib import asynccontextmanager +from datetime import datetime, timezone + +import httpx +import pytest + +from basic_memory.cli.commands.cloud.webdav import ( + WebdavError, + download_file, + etag_content_hash, + list_project_files, + normalize_etag, + upload_file, + webdav_path, +) + + +def _multistatus(self_href: str, entries: str) -> str: + return f""" + + + {self_href} + + + + self + + HTTP/1.1 200 OK + + +{entries} +""" + + +def _file_entry( + href: str, + name: str, + size: int, + *, + etag: str | None = '"d41d8cd98f00b204e9800998ecf8427e"', + modified: str | None = "Mon, 08 Jun 2026 10:30:00 GMT", +) -> str: + props = [ + "", + f"{name}", + f"{size}", + ] + if etag is not None: + props.append(f"{etag}") + if modified is not None: + props.append(f"{modified}") + joined = "".join(props) + return f""" + {href} + {joined}HTTP/1.1 200 OK +""" + + +def _dir_entry(href: str, name: str) -> str: + return f""" + {href} + + + + {name} + + HTTP/1.1 200 OK + +""" + + +@asynccontextmanager +async def _client(handler): + async with httpx.AsyncClient( + transport=httpx.MockTransport(handler), base_url="https://cloud.example.test" + ) as client: + yield client + + +def test_webdav_path_addresses_projects_and_files(): + assert webdav_path("research") == "/webdav/research" + assert webdav_path("research", "notes/a.md") == "/webdav/research/notes/a.md" + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + (None, None), + ('"abc"', "abc"), + (" abc ", "abc"), + ('W/"abc"', "W/abc"), + ('""', None), + ], +) +def test_normalize_etag(raw, expected): + assert normalize_etag(raw) == expected + + +@pytest.mark.parametrize( + ("etag", "expected"), + [ + (None, None), + ("D41D8CD98F00B204E9800998ECF8427E", "d41d8cd98f00b204e9800998ecf8427e"), + # Multipart digest-of-digests: not a content hash. + ("d41d8cd98f00b204e9800998ecf8427e-3", None), + # Weak validator: promises only semantic equivalence. + ("W/d41d8cd98f00b204e9800998ecf8427e", None), + ("opaque-tag", None), + ], +) +def test_etag_content_hash_only_accepts_single_part_digests(etag, expected): + assert etag_content_hash(etag) == expected + + +@pytest.mark.asyncio +async def test_list_project_files_walks_subdirectories(): + """The service lists one level at a time, so a full listing is a walk.""" + listings = { + "/webdav/research": _multistatus( + "/webdav/research/", + _file_entry("/webdav/research/top.md", "top.md", 4) + + _dir_entry("/webdav/research/notes/", "notes"), + ), + # Nested collections report only their basename in displayname, so the + # walk composes the relative path from the directory it is listing. + "/webdav/research/notes": _multistatus( + "/webdav/research/notes/", + _file_entry("/webdav/research/notes/deep.md", "deep.md", 9), + ), + } + seen: list[tuple[str, str]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + seen.append((request.method, request.url.path)) + assert request.headers["Depth"] == "1" + return httpx.Response(207, text=listings[request.url.path]) + + async with _client(handler) as client: + files = await list_project_files(client, "research") + + assert [f.path for f in files] == ["top.md", "notes/deep.md"] + assert all(method == "PROPFIND" for method, _ in seen) + assert [path for _, path in seen] == ["/webdav/research", "/webdav/research/notes"] + + top = files[0] + assert top.size == 4 + assert top.etag == "d41d8cd98f00b204e9800998ecf8427e" + assert top.modified == datetime(2026, 6, 8, 10, 30, tzinfo=timezone.utc) + + +@pytest.mark.asyncio +async def test_list_project_files_keeps_a_child_that_shadows_the_request_path(): + """Only the first response is the collection itself; a same-named child stays.""" + + async def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/webdav/research": + return httpx.Response( + 207, + text=_multistatus( + "/webdav/research/", _dir_entry("/webdav/research/notes/", "notes") + ), + ) + if request.url.path == "/webdav/research/notes": + # The service names a nested collection by its basename, so this + # child's href collides with the collection being listed. It is + # still a real child and its contents must not be dropped. + return httpx.Response( + 207, + text=_multistatus( + "/webdav/research/notes/", + _dir_entry("/webdav/research/notes/", "notes") + + _file_entry("/webdav/research/notes/a.md", "a.md", 1), + ), + ) + assert request.url.path == "/webdav/research/notes/notes" + return httpx.Response( + 207, + text=_multistatus( + "/webdav/research/notes/notes/", + _file_entry("/webdav/research/notes/notes/a.md", "a.md", 1), + ), + ) + + async with _client(handler) as client: + files = await list_project_files(client, "research") + + assert [f.path for f in files] == ["notes/a.md", "notes/notes/a.md"] + + +@pytest.mark.asyncio +async def test_list_project_files_falls_back_to_the_href_for_a_name(): + """A server that omits displayname is still usable: the href carries the name.""" + body = _multistatus( + "/webdav/research/", + """ + /webdav/research/with%20space.md + + 7 + +""", + ) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=body) + + async with _client(handler) as client: + files = await list_project_files(client, "research") + + assert [f.path for f in files] == ["with space.md"] + # No validators offered at all — the caller must decide, not assume. + assert files[0].etag is None + assert files[0].modified is None + + +@pytest.mark.asyncio +async def test_list_project_files_reports_an_entry_with_no_name(): + body = _multistatus( + "/webdav/research/", + "", + ) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=body) + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="no name"): + await list_project_files(client, "research") + + +@pytest.mark.asyncio +async def test_list_project_files_reports_unparseable_xml(): + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=" httpx.Response: + return httpx.Response(403, text="Forbidden") + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="HTTP 403"): + await list_project_files(client, "research") + + +@pytest.mark.asyncio +async def test_list_project_files_reports_a_non_numeric_size(): + body = _multistatus( + "/webdav/research/", + """ + /webdav/research/a.md + a.md + huge + +""", + ) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=body) + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="non-numeric file size"): + await list_project_files(client, "research") + + +@pytest.mark.asyncio +async def test_list_project_files_reports_an_unparseable_timestamp(): + body = _multistatus( + "/webdav/research/", + _file_entry("/webdav/research/a.md", "a.md", 1, modified="yesterday-ish"), + ) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=body) + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="unparseable timestamp"): + await list_project_files(client, "research") + + +@pytest.mark.asyncio +async def test_list_project_files_ignores_a_propstat_without_props(): + body = _multistatus( + "/webdav/research/", + """ + /webdav/research/a.md + HTTP/1.1 404 Not Found + a.md + 3 + +""", + ) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(207, text=body) + + async with _client(handler) as client: + files = await list_project_files(client, "research") + + assert [(f.path, f.size) for f in files] == [("a.md", 3)] + + +@pytest.mark.asyncio +async def test_download_file_returns_content_and_last_modified(): + async def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "GET" + assert request.url.path == "/webdav/research/notes/a.md" + return httpx.Response( + 200, + content=b"hello", + headers={"Last-Modified": "Mon, 08 Jun 2026 10:30:00 GMT"}, + ) + + async with _client(handler) as client: + downloaded = await download_file(client, "research", "notes/a.md") + + assert downloaded.content == b"hello" + assert downloaded.modified == datetime(2026, 6, 8, 10, 30, tzinfo=timezone.utc) + + +@pytest.mark.asyncio +async def test_download_file_reports_a_refused_download(): + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(404, text="File not found") + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="HTTP 404"): + await download_file(client, "research", "gone.md") + + +@pytest.mark.asyncio +async def test_upload_file_puts_content_with_the_local_mtime(): + seen: dict[str, object] = {} + + async def handler(request: httpx.Request) -> httpx.Response: + seen["method"] = request.method + seen["path"] = request.url.path + seen["mtime"] = request.headers["X-OC-Mtime"] + seen["content"] = request.content + return httpx.Response(201) + + async with _client(handler) as client: + await upload_file(client, "research", "notes/a.md", content=b"hi", mtime=1780000000) + + assert seen == { + "method": "PUT", + "path": "/webdav/research/notes/a.md", + "mtime": "1780000000", + "content": b"hi", + } + + +@pytest.mark.asyncio +async def test_upload_file_reports_a_refused_upload(): + """A viewer pushing to a Team project gets the service's refusal verbatim.""" + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(403, text="Editor access required") + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="Editor access required"): + await upload_file(client, "research", "a.md", content=b"hi", mtime=1) + + +@pytest.mark.asyncio +async def test_transport_errors_are_reported_without_a_response(): + async def handler(request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("connection refused") + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="connection refused"): + await download_file(client, "research", "a.md") + + +@pytest.mark.parametrize( + ("project", "rel_path", "expected"), + [ + # `#` and `?` are URL delimiters, so leaving them raw would truncate the + # request path at a perfectly legal filename character. + ("research", "a#draft.md", "/webdav/research/a%23draft.md"), + ("research", "b?v2.md", "/webdav/research/b%3Fv2.md"), + ("research", "notes/c d.md", "/webdav/research/notes/c%20d.md"), + ("my project", "a.md", "/webdav/my%20project/a.md"), + # Separators stay separators. + ("research", "one/two/three.md", "/webdav/research/one/two/three.md"), + ], +) +def test_webdav_path_percent_encodes_without_losing_separators(project, rel_path, expected): + assert webdav_path(project, rel_path) == expected + + +@pytest.mark.asyncio +async def test_delimiter_filenames_round_trip_through_list_and_download(): + """A note named `a#draft.md` must be listed, then fetched, as that same file.""" + listing = _multistatus( + "/webdav/research/", + _file_entry("/webdav/research/a%23draft.md", "a#draft.md", 4), + ) + fetched: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=listing) + fetched.append(request.url.path) + return httpx.Response(200, content=b"body") + + async with _client(handler) as client: + files = await list_project_files(client, "research") + await download_file(client, "research", files[0].path) + + assert [f.path for f in files] == ["a#draft.md"] + # httpx reports the decoded path; the delimiter survived the round trip. + assert fetched == ["/webdav/research/a#draft.md"] + + +@pytest.mark.asyncio +async def test_upload_file_create_only_sends_the_conditional_header(): + seen: dict[str, object] = {} + + async def handler(request: httpx.Request) -> httpx.Response: + seen["if_none_match"] = request.headers.get("If-None-Match") + return httpx.Response(201) + + async with _client(handler) as client: + written = await upload_file( + client, "research", "a.md", content=b"hi", mtime=1, create_only=True + ) + + assert written is True + assert seen == {"if_none_match": "*"} + + +@pytest.mark.asyncio +async def test_upload_file_create_only_reports_a_refused_precondition(): + """412 is the answer the request asked for, not a failure to raise on.""" + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(412, text="Precondition Failed") + + async with _client(handler) as client: + written = await upload_file( + client, "research", "a.md", content=b"hi", mtime=1, create_only=True + ) + + assert written is False + + +@pytest.mark.asyncio +async def test_upload_file_without_create_only_still_raises_on_412(): + """Only a conditional write can interpret 412; anywhere else it is an error.""" + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(412, text="Precondition Failed") + + async with _client(handler) as client: + with pytest.raises(WebdavError, match="HTTP 412"): + await upload_file(client, "research", "a.md", content=b"hi", mtime=1) diff --git a/tests/cli/cloud/test_webdav_transfer.py b/tests/cli/cloud/test_webdav_transfer.py new file mode 100644 index 000000000..0e5450a35 --- /dev/null +++ b/tests/cli/cloud/test_webdav_transfer.py @@ -0,0 +1,1107 @@ +"""Tests for the WebDAV push/pull engine used on Team workspaces (#1262). + +These exercise the real comparison and transfer code against a mocked WebDAV +surface, so every `--on-conflict` strategy is proven to behave the way it does on +the Personal (rclone) path. +""" + +import errno +import hashlib +import importlib +import os +import re +from contextlib import asynccontextmanager +from datetime import datetime, timezone +from pathlib import Path + +import httpx +import pytest + +from basic_memory.cli.commands.cloud.transfer import TransferPlan +from basic_memory.cli.commands.cloud.webdav import RemoteFile, WebdavError +from basic_memory.cli.commands.cloud.webdav_transfer import ( + build_transfer_plan, + scan_local_files, + webdav_project_diff, + webdav_project_transfer, +) +from basic_memory.ignore_utils import load_gitignore_patterns + +MODIFIED = datetime(2026, 6, 8, 10, 30, tzinfo=timezone.utc) + + +def _md5(data: bytes) -> str: + return hashlib.md5(data, usedforsecurity=False).hexdigest() + + +def _write(root: Path, rel_path: str, content: str, *, mtime: float | None = None) -> Path: + path = root / rel_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + if mtime is not None: + os.utime(path, (mtime, mtime)) + return path + + +# Sentinel for "derive the entity tag from the content", so an explicit None can +# still mean "the service reported no entity tag at all". +_DERIVE_ETAG = "derive" + + +def _remote(path: str, content: str, *, etag: str | None = _DERIVE_ETAG, modified=MODIFIED): + data = content.encode("utf-8") + return RemoteFile( + path=path, + size=len(data), + etag=_md5(data) if etag == _DERIVE_ETAG else etag, + modified=modified, + ) + + +def _plain(text: str) -> str: + """Strip the console's styling so assertions read against the words alone.""" + return re.sub(r"\x1b\[[0-9;]*m", "", text) + + +def _propfind_body(paths: list[str]) -> str: + """A minimal project listing naming exactly `paths` as files.""" + entries = "".join( + f"""/webdav/research/{path} + {path.split("/")[-1]} + 1 +""" + for path in paths + ) + return f""" + +/webdav/research/ + research + +{entries} +""" + + +def _client_factory(handler): + @asynccontextmanager + async def factory(): + async with httpx.AsyncClient( + transport=httpx.MockTransport(handler), base_url="https://cloud.example.test" + ) as client: + yield client + + return factory + + +# --- Planning --- + + +def test_plan_classifies_new_conflicting_and_destination_only_files(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "same.md", "identical") + _write(root, "diverged.md", "local version") + _write(root, "local-only.md", "mine") + + remote_files = [ + _remote("same.md", "identical"), + _remote("diverged.md", "cloud version!"), + _remote("cloud-only.md", "theirs"), + ] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.new == ["cloud-only.md"] + assert plan.conflicts == ["diverged.md"] + assert plan.dest_only == ["local-only.md"] + assert plan.errors == [] + + +def test_plan_direction_flips_which_side_is_the_source(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "local-only.md", "mine") + remote_files = [_remote("cloud-only.md", "theirs")] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="push") + + assert plan.new == ["local-only.md"] + assert plan.dest_only == ["cloud-only.md"] + + +def test_plan_treats_a_size_difference_as_a_conflict_without_hashing(config_home, tmp_path): + """Size settles it, so an unusable entity tag never even gets consulted.""" + root = tmp_path / "research" + _write(root, "a.md", "short") + remote_files = [RemoteFile(path="a.md", size=999, etag="opaque", modified=None)] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.conflicts == ["a.md"] + assert plan.errors == [] + + +def test_plan_ignores_bmignore_paths_on_both_sides(config_home, tmp_path): + """An ignored path is invisible in both listings, as rclone's filter makes it.""" + root = tmp_path / "research" + _write(root, ".hidden.md", "local hidden") + _write(root, "keep.md", "keep") + + remote_files = [_remote("keep.md", "keep"), _remote(".hidden.md", "cloud hidden")] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.new == [] + assert plan.conflicts == [] + assert plan.dest_only == [] + + +def test_scan_local_files_prunes_ignored_directories(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "notes/a.md", "a") + _write(root, ".git/config", "nope") + + patterns = load_gitignore_patterns(root, use_gitignore=False) + assert sorted(scan_local_files(root, patterns)) == ["notes/a.md"] + + +def test_plan_rejects_a_cloud_path_that_escapes_the_project(config_home, tmp_path): + root = tmp_path / "research" + root.mkdir() + remote_files = [RemoteFile(path="../escape.md", size=1, etag=None, modified=MODIFIED)] + + with pytest.raises(WebdavError, match="outside the project"): + build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + +# --- Comparison fallback when the entity tag cannot be a content hash --- + + +@pytest.mark.parametrize( + "etag", + [ + None, # server sent no validator + "d41d8cd98f00b204e9800998ecf8427e-4", # multipart digest-of-digests + "W/d41d8cd98f00b204e9800998ecf8427e", # weak validator + ], +) +def test_matching_size_and_timestamp_is_a_match_without_a_usable_etag(config_home, tmp_path, etag): + root = tmp_path / "research" + _write(root, "a.md", "same size", mtime=MODIFIED.timestamp()) + remote_files = [_remote("a.md", "same size", etag=etag)] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.conflicts == [] + assert plan.errors == [] + + +def test_a_diverged_timestamp_is_a_conflict_without_a_usable_etag(config_home, tmp_path): + """Erring toward a conflict is recoverable; a silent skip would lose an edit.""" + root = tmp_path / "research" + _write(root, "a.md", "same size", mtime=MODIFIED.timestamp() + 600) + remote_files = [_remote("a.md", "same size", etag=None)] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.conflicts == ["a.md"] + assert plan.errors == [] + + +def test_sub_second_clock_drift_is_still_a_match(config_home, tmp_path): + """HTTP-date has one-second resolution, so a fractional mtime still matches.""" + root = tmp_path / "research" + _write(root, "a.md", "same size", mtime=MODIFIED.timestamp() + 0.75) + remote_files = [_remote("a.md", "same size", etag=None)] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.conflicts == [] + + +def test_no_etag_and_no_timestamp_is_reported_as_uncomparable(config_home, tmp_path): + """With nothing to compare, the file goes to errors — never a silent match.""" + root = tmp_path / "research" + _write(root, "a.md", "same size") + remote_files = [RemoteFile(path="a.md", size=len("same size"), etag=None, modified=None)] + + plan = build_transfer_plan(local_root=root, remote_files=remote_files, direction="pull") + + assert plan.errors == ["a.md"] + assert plan.conflicts == [] + + +# --- Transfers --- + + +@pytest.mark.asyncio +async def test_pull_downloads_new_files_and_carries_the_cloud_timestamp(config_home, tmp_path): + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "GET" + assert request.url.path == "/webdav/research/notes/new.md" + return httpx.Response( + 200, + content=b"from cloud", + headers={"Last-Modified": "Mon, 08 Jun 2026 10:30:00 GMT"}, + ) + + plan = TransferPlan(new=["notes/new.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + target = root / "notes" / "new.md" + assert target.read_bytes() == b"from cloud" + # rclone preserves modtimes across a transfer; so does this, which is what + # keeps the timestamp fallback in _compare meaningful after a pull. + assert target.stat().st_mtime == pytest.approx(MODIFIED.timestamp()) + # The atomic write leaves nothing behind. + assert sorted(p.name for p in (root / "notes").iterdir()) == ["new.md"] + + +@pytest.mark.asyncio +async def test_pull_default_never_overwrites_an_existing_local_file(config_home, tmp_path): + """With no conflicts to resolve, only new files move — the additive contract.""" + root = tmp_path / "research" + _write(root, "kept.md", "local wins") + + requested: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + requested.append(request.url.path) + return httpx.Response(200, content=b"from cloud") + + plan = TransferPlan(new=["new.md"], conflicts=["kept.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + strategy="keep-local", + client_cm_factory=_client_factory(handler), + ) + + assert requested == ["/webdav/research/new.md"] + assert (root / "kept.md").read_text() == "local wins" + + +@pytest.mark.asyncio +async def test_pull_keep_cloud_overwrites_the_conflicting_local_file(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "dup.md", "local version") + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, content=b"cloud version") + + plan = TransferPlan(conflicts=["dup.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + strategy="keep-cloud", + client_cm_factory=_client_factory(handler), + ) + + assert (root / "dup.md").read_text() == "cloud version" + + +@pytest.mark.asyncio +async def test_pull_keep_both_writes_the_incoming_copy_beside_the_local_one(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "notes/dup.md", "local version") + + async def handler(request: httpx.Request) -> httpx.Response: + # keep-both fetches the conflicting file under its real name and lands it + # under the conflict name, so nothing is lost on either side. + assert request.url.path == "/webdav/research/notes/dup.md" + return httpx.Response(200, content=b"cloud version") + + plan = TransferPlan(conflicts=["notes/dup.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + strategy="keep-both", + conflict_suffix="20260608-1030", + client_cm_factory=_client_factory(handler), + ) + + assert (root / "notes" / "dup.md").read_text() == "local version" + conflict_copy = root / "notes" / "dup.conflict-20260608-1030.md" + assert conflict_copy.read_text() == "cloud version" + + +@pytest.mark.asyncio +async def test_push_uploads_new_files_with_their_local_mtime(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "notes/new.md", "local content", mtime=1780000000) + + seen: list[tuple[str, bytes, str]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + seen.append((request.url.path, request.content, request.headers["X-OC-Mtime"])) + return httpx.Response(201) + + plan = TransferPlan(new=["notes/new.md"]) + await webdav_project_transfer( + "research", + root, + "push", + plan, + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert seen == [("/webdav/research/notes/new.md", b"local content", "1780000000")] + + +@pytest.mark.asyncio +async def test_push_keep_local_overwrites_the_conflicting_cloud_file(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "dup.md", "local wins") + + seen: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + seen.append(request.url.path) + return httpx.Response(204) + + plan = TransferPlan(conflicts=["dup.md"]) + await webdav_project_transfer( + "research", + root, + "push", + plan, + workspace_id="team-tenant", + strategy="keep-local", + client_cm_factory=_client_factory(handler), + ) + + assert seen == ["/webdav/research/dup.md"] + + +@pytest.mark.asyncio +async def test_push_keep_cloud_leaves_the_conflicting_cloud_file_alone(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "dup.md", "local version") + _write(root, "new.md", "new") + + seen: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + seen.append(request.url.path) + return httpx.Response(201) + + plan = TransferPlan(new=["new.md"], conflicts=["dup.md"]) + await webdav_project_transfer( + "research", + root, + "push", + plan, + workspace_id="team-tenant", + strategy="keep-cloud", + client_cm_factory=_client_factory(handler), + ) + + assert seen == ["/webdav/research/new.md"] + + +@pytest.mark.asyncio +async def test_push_keep_both_uploads_the_incoming_copy_under_a_conflict_name( + config_home, tmp_path +): + root = tmp_path / "research" + _write(root, "notes/dup.md", "local version") + + seen: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + seen.append(request.url.path) + return httpx.Response(201) + + plan = TransferPlan(conflicts=["notes/dup.md"]) + await webdav_project_transfer( + "research", + root, + "push", + plan, + workspace_id="team-tenant", + strategy="keep-both", + conflict_suffix="20260608-1030", + client_cm_factory=_client_factory(handler), + ) + + # The cloud's own copy is untouched; the local version lands beside it. + assert seen == ["/webdav/research/notes/dup.conflict-20260608-1030.md"] + + +@pytest.mark.asyncio +async def test_transfer_dry_run_moves_nothing(config_home, tmp_path, capsys): + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: # pragma: no cover + raise AssertionError("dry run must not touch the network") + + plan = TransferPlan(new=["a.md"], conflicts=["dup.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + strategy="keep-both", + conflict_suffix="S", + dry_run=True, + client_cm_factory=_client_factory(handler), + ) + + output = " ".join(_plain(capsys.readouterr().out).split()) + assert "2 file(s) would be transferred" in output + assert "dup.md -> dup.conflict-S.md" in output + assert not list(root.iterdir()) + + +@pytest.mark.asyncio +async def test_transfer_reports_when_there_is_nothing_to_do(config_home, tmp_path, capsys): + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: # pragma: no cover + raise AssertionError("nothing to transfer must not touch the network") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(dest_only=["local-only.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert "Nothing to transfer" in _plain(capsys.readouterr().out) + + +@pytest.mark.asyncio +async def test_transfer_verbose_lists_each_file(config_home, tmp_path, capsys): + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, content=b"x") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["a.md"]), + workspace_id="team-tenant", + verbose=True, + client_cm_factory=_client_factory(handler), + ) + + output = _plain(capsys.readouterr().out) + assert "a.md" in output + assert "Transferred 1 file(s)" in output + + +@pytest.mark.asyncio +async def test_transfer_stops_on_a_refused_upload(config_home, tmp_path): + """A viewer pushing to a Team project fails loudly rather than half-succeeding.""" + root = tmp_path / "research" + _write(root, "a.md", "content") + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(403, text="Editor access required") + + with pytest.raises(WebdavError, match="Editor access required"): + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(new=["a.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + +# --- Diff over the wire --- + + +@pytest.mark.asyncio +async def test_webdav_project_diff_lists_the_cloud_and_compares(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "same.md", "identical") + + body = f""" + +/webdav/research/ + research + +/webdav/research/same.md + same.md + 9 + "{_md5(b"identical")}" + Mon, 08 Jun 2026 10:30:00 GMT + +/webdav/research/theirs.md + theirs.md + 6 + "{_md5(b"theirs")}" + Mon, 08 Jun 2026 10:30:00 GMT + +""" + + async def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "PROPFIND" + return httpx.Response(207, text=body) + + plan = await webdav_project_diff( + "research", + root, + "pull", + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert plan.new == ["theirs.md"] + assert plan.conflicts == [] + assert plan.dest_only == [] + + +# --- Symlinks never let a transfer leave the project boundary --- + + +def test_scan_skips_symlinked_files(config_home, tmp_path): + """Push must not read bytes from outside the project through a link.""" + outside = tmp_path / "outside.md" + outside.write_text("secret", encoding="utf-8") + root = tmp_path / "research" + _write(root, "real.md", "real") + (root / "link.md").symlink_to(outside) + + patterns = load_gitignore_patterns(root, use_gitignore=False) + assert sorted(scan_local_files(root, patterns)) == ["real.md"] + + +def test_scan_does_not_descend_into_symlinked_directories(config_home, tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.md").write_text("secret", encoding="utf-8") + root = tmp_path / "research" + _write(root, "real.md", "real") + (root / "linked").symlink_to(outside, target_is_directory=True) + + patterns = load_gitignore_patterns(root, use_gitignore=False) + assert sorted(scan_local_files(root, patterns)) == ["real.md"] + + +@pytest.mark.asyncio +async def test_pull_refuses_to_write_through_a_symlinked_directory(config_home, tmp_path): + """A lexically clean path can still point outside once a link is resolved.""" + outside = tmp_path / "outside" + outside.mkdir() + root = tmp_path / "research" + root.mkdir() + (root / "notes").symlink_to(outside, target_is_directory=True) + + async def handler(request: httpx.Request) -> httpx.Response: # pragma: no cover + raise AssertionError("must refuse before touching the network") + + with pytest.raises(WebdavError, match="link out of the project"): + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["notes/planted.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert not (outside / "planted.md").exists() + + +@pytest.mark.asyncio +async def test_push_refuses_to_read_through_a_symlinked_directory(config_home, tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.md").write_text("secret", encoding="utf-8") + root = tmp_path / "research" + root.mkdir() + (root / "notes").symlink_to(outside, target_is_directory=True) + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + raise AssertionError("must refuse before uploading anything") + + with pytest.raises(WebdavError, match="link out of the project"): + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(new=["notes/secret.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + +@pytest.mark.asyncio +async def test_pull_keep_cloud_refuses_to_overwrite_a_symlinked_note(config_home, tmp_path): + outside = tmp_path / "outside.md" + outside.write_text("original", encoding="utf-8") + root = tmp_path / "research" + root.mkdir() + (root / "dup.md").symlink_to(outside) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, content=b"cloud version") + + with pytest.raises(WebdavError, match="symlinked path"): + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(conflicts=["dup.md"]), + workspace_id="team-tenant", + strategy="keep-cloud", + client_cm_factory=_client_factory(handler), + ) + + assert outside.read_text() == "original" + + +# --- A path planned as new is created, never used to replace --- + + +@pytest.mark.asyncio +async def test_pull_leaves_a_local_note_that_appeared_after_planning(config_home, tmp_path, capsys): + """The plan is a snapshot; the exclusive create is what makes acting on it safe.""" + root = tmp_path / "research" + root.mkdir() + # Written after the plan classified this path as new — a note nobody compared. + _write(root, "raced.md", "written since the plan") + + async def handler(request: httpx.Request) -> httpx.Response: + # The download does run — nothing may be staged at the destination until + # the bytes exist — but publication is what refuses. + return httpx.Response(200, content=b"from cloud") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["raced.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert (root / "raced.md").read_text() == "written since the plan" + # The staged temp file is cleaned up either way. + assert sorted(p.name for p in root.iterdir()) == ["raced.md"] + output = _plain(capsys.readouterr().out) + assert "Transferred 0 file(s)" in output + assert "appeared on the destination" in output + assert "raced.md" in output + + +@pytest.mark.asyncio +async def test_push_leaves_a_cloud_note_that_appeared_after_planning(config_home, tmp_path, capsys): + """Mirrors rclone's --ignore-existing, which re-reads the destination at copy time.""" + root = tmp_path / "research" + _write(root, "raced.md", "local content") + _write(root, "fresh.md", "also local") + + listing = _propfind_body(["raced.md"]) + puts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=listing) + puts.append(request.url.path) + return httpx.Response(201) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(new=["fresh.md", "raced.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert puts == ["/webdav/research/fresh.md"] + output = _plain(capsys.readouterr().out) + assert "Transferred 1 file(s)" in output + assert "appeared on the destination" in output + assert "raced.md" in output + + +@pytest.mark.asyncio +async def test_push_keep_local_still_overwrites_a_resolved_conflict(config_home, tmp_path): + """An explicit resolution is an instruction to overwrite, not a stale guess.""" + root = tmp_path / "research" + _write(root, "dup.md", "local wins") + + puts: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body(["dup.md"])) + puts.append(request.url.path) + return httpx.Response(204) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(conflicts=["dup.md"]), + workspace_id="team-tenant", + strategy="keep-local", + client_cm_factory=_client_factory(handler), + ) + + assert puts == ["/webdav/research/dup.md"] + + +@pytest.mark.asyncio +async def test_pull_keep_cloud_still_overwrites_a_resolved_conflict(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "dup.md", "local version") + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, content=b"cloud version") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(conflicts=["dup.md"]), + workspace_id="team-tenant", + strategy="keep-cloud", + client_cm_factory=_client_factory(handler), + ) + + assert (root / "dup.md").read_text() == "cloud version" + + +@pytest.mark.asyncio +async def test_pull_leaves_no_placeholder_when_the_download_fails(config_home, tmp_path): + """The exclusive create must not survive as an empty phantom note.""" + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(500, text="boom") + + with pytest.raises(WebdavError, match="HTTP 500"): + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["a.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert not (root / "a.md").exists() + + +# --- Filenames that are legal on disk but structural in a URL --- + + +@pytest.mark.asyncio +async def test_pull_round_trips_filenames_containing_url_delimiters(config_home, tmp_path): + """`#` and `?` are path data here, not a fragment and a query.""" + root = tmp_path / "research" + root.mkdir() + requested: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + # httpx decodes the path it received; the delimiters must survive intact. + requested.append(request.url.path) + return httpx.Response(200, content=b"body") + + # `?` is exercised in the client tests instead: Windows will not allow a file + # by that name on disk, and this test has to actually write one. + plan = TransferPlan(new=["notes/a#draft.md", "notes/c d.md"]) + await webdav_project_transfer( + "research", + root, + "pull", + plan, + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert requested == [ + "/webdav/research/notes/a#draft.md", + "/webdav/research/notes/c d.md", + ] + assert (root / "notes" / "a#draft.md").read_bytes() == b"body" + assert (root / "notes" / "c d.md").read_bytes() == b"body" + + +@pytest.mark.asyncio +async def test_push_sends_filenames_containing_url_delimiters_intact(config_home, tmp_path): + root = tmp_path / "research" + _write(root, "a#draft.md", "content") + requested: list[str] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + requested.append(request.url.path) + return httpx.Response(201) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(new=["a#draft.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert requested == ["/webdav/research/a#draft.md"] + + +# --- Nothing is created at the destination before the bytes exist --- + + +@pytest.mark.asyncio +async def test_pull_leaves_a_note_created_during_the_download(config_home, tmp_path, capsys): + """The destination is claimed after the download, so a note that lands mid-flight survives.""" + root = tmp_path / "research" + root.mkdir() + + async def handler(request: httpx.Request) -> httpx.Response: + # Racing writer: the note appears while the download is in flight. + _write(root, "raced.md", "written during the download") + return httpx.Response(200, content=b"from cloud") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["raced.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert (root / "raced.md").read_text() == "written during the download" + output = _plain(capsys.readouterr().out) + assert "Transferred 0 file(s)" in output + assert "appeared on the destination" in output + + +@pytest.mark.asyncio +async def test_pull_leaves_content_written_into_the_destination_during_the_download( + config_home, tmp_path +): + """No empty placeholder exists for an editor to fill and have discarded.""" + root = tmp_path / "research" + root.mkdir() + observed: list[list[str]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + # An editor opening the destination mid-download must find nothing there + # to open, and its content must survive whatever the transfer does next. + observed.append(sorted(p.name for p in root.iterdir())) + _write(root, "raced.md", "editor content") + return httpx.Response(200, content=b"from cloud") + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["raced.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert observed == [[]] # nothing staged at the destination before the bytes existed + assert (root / "raced.md").read_text() == "editor content" + + +@pytest.mark.asyncio +async def test_pull_leaves_nothing_behind_when_publication_fails( + config_home, tmp_path, monkeypatch +): + """A failure after a successful download must not leave a phantom note.""" + root = tmp_path / "research" + root.mkdir() + + module = importlib.import_module("basic_memory.cli.commands.cloud.webdav_transfer") + + def _explode(*_args, **_kwargs): + raise OSError("publication failed") + + monkeypatch.setattr(module.os, "link", _explode) + monkeypatch.setattr(module.os, "open", _explode) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, content=b"from cloud") + + with pytest.raises(OSError, match="publication failed"): + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["a.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert sorted(p.name for p in root.iterdir()) == [] + + +@pytest.mark.asyncio +async def test_pull_publishes_without_hardlinks_when_the_filesystem_cannot( + config_home, tmp_path, monkeypatch +): + """exFAT and some virtual mounts have no hardlinks; the no-clobber rule still holds.""" + root = tmp_path / "research" + root.mkdir() + _write(root, "taken.md", "already here") + + module = importlib.import_module("basic_memory.cli.commands.cloud.webdav_transfer") + + def _unsupported(*_args, **_kwargs): + raise OSError(errno.EPERM, "hardlinks are not supported here") + + monkeypatch.setattr(module.os, "link", _unsupported) + + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, + content=b"from cloud", + headers={"Last-Modified": "Mon, 08 Jun 2026 10:30:00 GMT"}, + ) + + await webdav_project_transfer( + "research", + root, + "pull", + TransferPlan(new=["fresh.md", "taken.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + # The new name is written, timestamp and all; the taken one is untouched. + assert (root / "fresh.md").read_bytes() == b"from cloud" + assert (root / "fresh.md").stat().st_mtime == pytest.approx(MODIFIED.timestamp()) + assert (root / "taken.md").read_text() == "already here" + + +# --- The conditional create, not the re-list, is what holds the line on push --- + + +@pytest.mark.asyncio +async def test_push_sends_a_conditional_create_and_honors_a_refusal(config_home, tmp_path, capsys): + """A path created after the re-list is refused at the write itself.""" + root = tmp_path / "research" + _write(root, "raced.md", "local content") + _write(root, "fresh.md", "also local") + + seen: list[tuple[str, str | None]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + # The listing is clean: this race opens *after* it. + return httpx.Response(207, text=_propfind_body([])) + seen.append((request.url.path, request.headers.get("If-None-Match"))) + if request.url.path.endswith("raced.md"): + return httpx.Response(412, text="Precondition Failed") + return httpx.Response(201) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(new=["fresh.md", "raced.md"]), + workspace_id="team-tenant", + client_cm_factory=_client_factory(handler), + ) + + assert seen == [ + ("/webdav/research/fresh.md", "*"), + ("/webdav/research/raced.md", "*"), + ] + output = _plain(capsys.readouterr().out) + assert "Transferred 1 file(s)" in output + assert "appeared on the destination" in output + assert "raced.md" in output + + +@pytest.mark.asyncio +async def test_push_keep_local_does_not_send_a_conditional_create(config_home, tmp_path): + """An explicit resolution is an instruction to replace, so it must not be refused.""" + root = tmp_path / "research" + _write(root, "dup.md", "local wins") + + seen: list[tuple[str, str | None]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body(["dup.md"])) + seen.append((request.url.path, request.headers.get("If-None-Match"))) + return httpx.Response(204) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(conflicts=["dup.md"]), + workspace_id="team-tenant", + strategy="keep-local", + client_cm_factory=_client_factory(handler), + ) + + assert seen == [("/webdav/research/dup.md", None)] + + +@pytest.mark.asyncio +async def test_push_keep_both_conflict_copies_are_conditional(config_home, tmp_path): + """A conflict copy is a create too — it must never land on an existing name.""" + root = tmp_path / "research" + _write(root, "dup.md", "local version") + + seen: list[tuple[str, str | None]] = [] + + async def handler(request: httpx.Request) -> httpx.Response: + if request.method == "PROPFIND": + return httpx.Response(207, text=_propfind_body([])) + seen.append((request.url.path, request.headers.get("If-None-Match"))) + return httpx.Response(201) + + await webdav_project_transfer( + "research", + root, + "push", + TransferPlan(conflicts=["dup.md"]), + workspace_id="team-tenant", + strategy="keep-both", + conflict_suffix="S", + client_cm_factory=_client_factory(handler), + ) + + assert seen == [("/webdav/research/dup.conflict-S.md", "*")] diff --git a/tests/test_rclone_commands.py b/tests/test_rclone_commands.py index d3a6c7c39..fd93aefee 100644 --- a/tests/test_rclone_commands.py +++ b/tests/test_rclone_commands.py @@ -6,12 +6,14 @@ import pytest +from basic_memory.cli.commands.cloud.transfer import ( + TransferPlan, + conflict_copy_name, +) from basic_memory.cli.commands.cloud.rclone_commands import ( MIN_RCLONE_VERSION_EMPTY_DIRS, RcloneError, SyncProject, - TransferPlan, - _conflict_copy_name, _parse_check_combined, bisync_initialized, check_rclone_installed, @@ -607,8 +609,8 @@ def test_parse_check_combined_handles_paths_with_spaces(): def test_conflict_copy_name_inserts_marker_before_extension(): - assert _conflict_copy_name("notes/x.md", "20260608-1030") == "notes/x.conflict-20260608-1030.md" - assert _conflict_copy_name("top.md", "S") == "top.conflict-S.md" + assert conflict_copy_name("notes/x.md", "20260608-1030") == "notes/x.conflict-20260608-1030.md" + assert conflict_copy_name("top.md", "S") == "top.conflict-S.md" def test_project_diff_pull_uses_remote_as_source(tmp_path):