From 85845272a7c501592ffc5c14e82844d0eced576d Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Sat, 15 Aug 2026 16:14:32 -0500 Subject: [PATCH 1/3] fix(cli): run Team push/pull over WebDAV instead of storage credentials `bm cloud pull` / `bm cloud push` could not run at all on organization (Team) workspaces. Both required a tenant-scoped rclone remote, and provisioning one mints object-storage credentials through an owner-only endpoint, so every other member was told to run `bm cloud setup` and got a 403 from it. Widening that endpoint is the wrong fix: storage credentials are scoped to a whole tenant bucket, so they cannot express per-project access. Team transfers now use the service's WebDAV surface, which authorizes each request against the caller's access to that project. - webdav.py adds the read half of the protocol (PROPFIND listing, GET) beside the PUT that `bm cloud upload` already used, including the entity tag and last-modified validators a transfer needs. - webdav_transfer.py compares by entity tag plus size, falling back to last-modified plus size when the tag cannot be a content hash (absent, weak, or multipart shaped), and reports a file as uncomparable rather than guessing when neither is available. - transfer.py holds the plan vocabulary both transports share. The CLI surface is unchanged: same commands, same flags, same `--on-conflict` default of `fail`, still additive in both directions. `bisync` remains gated to Personal workspaces, and the Personal rclone path is untouched. Depends on the companion cloud change that returns `getetag` and `getlastmodified` from PROPFIND and ETag / Last-Modified on GET. Refs #1262 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Drew Cain --- .../cli/commands/cloud/project_sync.py | 225 +++++-- .../cli/commands/cloud/rclone_commands.py | 70 +-- .../cli/commands/cloud/transfer.py | 58 ++ src/basic_memory/cli/commands/cloud/upload.py | 6 +- src/basic_memory/cli/commands/cloud/webdav.py | 361 +++++++++++ .../cli/commands/cloud/webdav_transfer.py | 367 ++++++++++++ tests/cli/cloud/test_project_sync_command.py | 214 ++++++- tests/cli/cloud/test_webdav_client.py | 385 ++++++++++++ tests/cli/cloud/test_webdav_transfer.py | 559 ++++++++++++++++++ tests/test_rclone_commands.py | 10 +- 10 files changed, 2118 insertions(+), 137 deletions(-) create mode 100644 src/basic_memory/cli/commands/cloud/transfer.py create mode 100644 src/basic_memory/cli/commands/cloud/webdav.py create mode 100644 src/basic_memory/cli/commands/cloud/webdav_transfer.py create mode 100644 tests/cli/cloud/test_webdav_client.py create mode 100644 tests/cli/cloud/test_webdav_transfer.py 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..41fdaa162 --- /dev/null +++ b/src/basic_memory/cli/commands/cloud/webdav.py @@ -0,0 +1,361 @@ +"""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 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 path unescaped; httpx percent-encodes it on the way out. + """ + if not rel_path: + return f"{WEBDAV_ROOT}/{project}" + return f"{WEBDAV_ROOT}/{project}/{rel_path}" + + +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, +) -> None: + """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. + + Raises: + WebdavError: If the service refuses the upload. + """ + request_path = webdav_path(project, rel_path) + try: + response = await client.put( + request_path, + content=content, + headers={"X-OC-Mtime": str(mtime)}, + ) + response.raise_for_status() + except httpx.HTTPError as exc: + raise WebdavError(f"Failed to upload {rel_path}: {_describe(exc)}") from exc + + +# --- 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.""" + return _href_path(href).rstrip("/") == 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..b971364dd --- /dev/null +++ b/src/basic_memory/cli/commands/cloud/webdav_transfer.py @@ -0,0 +1,367 @@ +"""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 +- 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. +""" + +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 ( + 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 + + +# --- 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 = ( + [(rel_path, conflict_copy_name(rel_path, conflict_suffix)) for rel_path in plan.conflicts] + if strategy == "keep-both" + else [] + ) + + overwrite = strategy_overwrites_dest(direction, strategy) + copies = [(rel_path, rel_path) for rel_path in plan.new] + if overwrite: + copies.extend((rel_path, rel_path) 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 source_rel, dest_rel in transfers: + suffix = "" if source_rel == dest_rel else f" -> {dest_rel}" + console.print(f" [dim]{source_rel}{suffix}[/dim]") + return + + cm_factory = client_cm_factory or partial(get_cloud_proxy_client, workspace=workspace_id) + async with cm_factory() as client: + for source_rel, dest_rel in transfers: + if verbose: + suffix = "" if source_rel == dest_rel else f" -> {dest_rel}" + console.print(f" {source_rel}{suffix}") + if direction == "pull": + await _pull_file(client, project, local_root, source_rel, dest_rel) + else: + await _push_file(client, project, local_root, source_rel, dest_rel) + + console.print(f"[dim]Transferred {len(transfers)} file(s).[/dim]") + + +# --- 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. + """ + files: dict[str, LocalFile] = {} + + for root, dirs, filenames in os.walk(local_root): + root_path = Path(root) + dirs[:] = [ + name + for name in dirs + if not should_ignore_path(root_path / name, local_root, ignore_patterns) + ] + + for filename in filenames: + file_path = root_path / filename + 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() + + +# --- Single-file transfers --- + + +async def _pull_file( + client: httpx.AsyncClient, + project: str, + local_root: Path, + source_rel: str, + dest_rel: str, +) -> None: + """Download one cloud file and land it under the destination path.""" + target = _safe_local_path(local_root, dest_rel) + downloaded = await download_file(client, project, source_rel) + + target.parent.mkdir(parents=True, exist_ok=True) + # Write to a sibling temp file and rename into place: an interrupted pull + # must never leave a half-written note in the user's knowledge base. + 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) + # 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 not None: + stamp = downloaded.modified.timestamp() + os.utime(temp_path, (stamp, stamp)) + os.replace(temp_path, target) + finally: + # A no-op once the rename succeeded; the cleanup path when it did not. + temp_path.unlink(missing_ok=True) + + +async def _push_file( + client: httpx.AsyncClient, + project: str, + local_root: Path, + source_rel: str, + dest_rel: str, +) -> None: + """Upload one local file to the destination path in the cloud project.""" + source = _safe_local_path(local_root, source_rel) + stat = source.stat() + await upload_file( + client, + project, + dest_rel, + content=source.read_bytes(), + mtime=int(stat.st_mtime), + ) + + +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. + + 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}") + return local_root.joinpath(*candidate.parts) 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..15861f23e --- /dev/null +++ b/tests/cli/cloud/test_webdav_client.py @@ -0,0 +1,385 @@ +"""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") diff --git a/tests/cli/cloud/test_webdav_transfer.py b/tests/cli/cloud/test_webdav_transfer.py new file mode 100644 index 000000000..b6d55b6c7 --- /dev/null +++ b/tests/cli/cloud/test_webdav_transfer.py @@ -0,0 +1,559 @@ +"""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 hashlib +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 _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: + 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: + 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: + 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: + 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 == [] 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): From 0408031ec1eae34915e16c38ba41e8dfa861f01b Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Sat, 15 Aug 2026 16:55:21 -0500 Subject: [PATCH 2/3] fix(cli): close symlink, encoding, and stale-plan gaps in WebDAV transfer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the Team push/pull transport. Symlinks were not handled at all. `_safe_local_path` was a lexical check, which cannot see a link: a symlinked file or parent directory 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 local scan now skips symlinked files and does not descend into symlinked directories, matching what the local project scanner does for the same reason, and containment is enforced on the resolved parent chain rather than the lexical path. A link at the final component is refused where a transfer would follow it — reading a push source, or replacing a file resolved with keep-cloud. Request paths were not percent-encoded, on the assumption that the HTTP client would handle it. That does not hold for `?` and `#`, which are structural URL delimiters rather than path data: a note named `a#draft.md` was requested as `a`. Both are legal POSIX filenames. A path classified as `new` was transferred unconditionally, so a file that appeared on the destination between planning and writing was destroyed — pull replaced a newly created local note, push overwrote a teammate's, 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`, which rclone evaluates against the destination listing it makes at copy time. Create-only transfers now get the same property. Pull claims the path with an exclusive create, which is atomic and also refuses a link. Push re-reads the cloud immediately before transferring and drops any create-only path that now exists; that narrows the window rather than closing it, because the write endpoint does not act on conditional request headers, so an `If-None-Match: *` guard would silently do nothing. Files left alone are named in the output rather than skipped quietly, so a re-run compares them as conflicts. Refs #1262 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Drew Cain --- src/basic_memory/cli/commands/cloud/webdav.py | 22 +- .../cli/commands/cloud/webdav_transfer.py | 210 ++++++++++-- tests/cli/cloud/test_webdav_client.py | 41 +++ tests/cli/cloud/test_webdav_transfer.py | 318 ++++++++++++++++++ 4 files changed, 559 insertions(+), 32 deletions(-) diff --git a/src/basic_memory/cli/commands/cloud/webdav.py b/src/basic_memory/cli/commands/cloud/webdav.py index 41fdaa162..baf0533fc 100644 --- a/src/basic_memory/cli/commands/cloud/webdav.py +++ b/src/basic_memory/cli/commands/cloud/webdav.py @@ -24,7 +24,7 @@ from dataclasses import dataclass from datetime import datetime from email.utils import parsedate_to_datetime -from urllib.parse import unquote, urlsplit +from urllib.parse import quote, unquote, urlsplit import httpx @@ -86,11 +86,16 @@ class _Entry: 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 path unescaped; httpx percent-encodes it on the way out. + 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}/{project}" - return f"{WEBDAV_ROOT}/{project}/{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: @@ -350,8 +355,13 @@ def _href_path(href: str) -> str: def _same_path(href: str, request_path: str) -> bool: - """Compare an href against a request path, ignoring a trailing slash.""" - return _href_path(href).rstrip("/") == request_path.rstrip("/") + """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: diff --git a/src/basic_memory/cli/commands/cloud/webdav_transfer.py b/src/basic_memory/cli/commands/cloud/webdav_transfer.py index b971364dd..fe5ee8c31 100644 --- a/src/basic_memory/cli/commands/cloud/webdav_transfer.py +++ b/src/basic_memory/cli/commands/cloud/webdav_transfer.py @@ -14,11 +14,15 @@ - 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 - 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. +``_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 @@ -79,6 +83,25 @@ class LocalFile: 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 --- @@ -135,15 +158,22 @@ async def webdav_project_transfer( # 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 = ( - [(rel_path, conflict_copy_name(rel_path, conflict_suffix)) for rel_path in plan.conflicts] + [ + _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 = [(rel_path, rel_path) for rel_path in plan.new] + copies = [_Transfer(rel_path, rel_path, create_only=True) for rel_path in plan.new] if overwrite: - copies.extend((rel_path, rel_path) for rel_path in plan.conflicts) + copies.extend( + _Transfer(rel_path, rel_path, create_only=False) for rel_path in plan.conflicts + ) transfers = renames + copies if not transfers: @@ -152,23 +182,32 @@ async def webdav_project_transfer( if dry_run: console.print(f"[dim]Dry run: {len(transfers)} file(s) would be transferred.[/dim]") - for source_rel, dest_rel in transfers: - suffix = "" if source_rel == dest_rel else f" -> {dest_rel}" - console.print(f" [dim]{source_rel}{suffix}[/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: - for source_rel, dest_rel in transfers: + if direction == "push": + transfers, appeared = await _drop_appeared_on_cloud(client, project, transfers) + else: + appeared = [] + + transferred = 0 + for transfer in transfers: if verbose: - suffix = "" if source_rel == dest_rel else f" -> {dest_rel}" - console.print(f" {source_rel}{suffix}") + console.print(f" {transfer.describe()}") if direction == "pull": - await _pull_file(client, project, local_root, source_rel, dest_rel) + written = await _pull_file(client, project, local_root, transfer) + if not written: + appeared.append(transfer.dest_rel) + continue else: - await _push_file(client, project, local_root, source_rel, dest_rel) + await _push_file(client, project, local_root, transfer) + transferred += 1 - console.print(f"[dim]Transferred {len(transfers)} file(s).[/dim]") + console.print(f"[dim]Transferred {transferred} file(s).[/dim]") + _report_appeared(appeared) # --- Planning --- @@ -227,19 +266,26 @@ def scan_local_files(local_root: Path, ignore_patterns: set[str]) -> dict[str, L 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): + for root, dirs, filenames in os.walk(local_root, followlinks=False): root_path = Path(root) dirs[:] = [ name for name in dirs - if not should_ignore_path(root_path / name, local_root, ignore_patterns) + 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() @@ -296,6 +342,63 @@ def _file_content_hash(path: Path) -> str: 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. These two +# guards give the WebDAV path the same property, per direction. + + +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. + + This is the analogue of rclone re-listing the destination at copy time. It + narrows the window rather than closing it: nothing here is atomic with the + PUT that follows, so a file created in the remaining moments is still + overwritten. Closing it needs a conditional create on the service side; the + write endpoint does not act on conditional request headers today, so sending + one would be a guard that silently does nothing. + """ + 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 --- @@ -303,14 +406,38 @@ async def _pull_file( client: httpx.AsyncClient, project: str, local_root: Path, - source_rel: str, - dest_rel: str, -) -> None: - """Download one cloud file and land it under the destination path.""" - target = _safe_local_path(local_root, dest_rel) - downloaded = await download_file(client, project, source_rel) + transfer: _Transfer, +) -> bool: + """Download one cloud file and land it under the destination path. + Returns False when a create-only transfer found the path 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) target.parent.mkdir(parents=True, exist_ok=True) + + # Trigger: the plan cleared this path for creation only. + # Why: an exclusive create is the one check the filesystem can make + # atomically — a stat-then-write would leave the same race open. It also + # refuses a symlink outright, so a link that appeared here is treated as the + # path being taken rather than followed. + # Outcome: claim the name up front, or leave the existing note alone. + if transfer.create_only: + try: + os.close(os.open(target, os.O_CREAT | os.O_EXCL | os.O_WRONLY)) + except FileExistsError: + return False + else: + _refuse_symlink(target, transfer.dest_rel) + + try: + downloaded = await download_file(client, project, transfer.source_rel) + except WebdavError: + # Do not leave the empty placeholder behind as a phantom note. + if transfer.create_only: + target.unlink(missing_ok=True) + raise + # Write to a sibling temp file and rename into place: an interrupted pull # must never leave a half-written note in the user's knowledge base. handle, temp_name = tempfile.mkstemp( @@ -331,21 +458,23 @@ async def _pull_file( # A no-op once the rename succeeded; the cleanup path when it did not. temp_path.unlink(missing_ok=True) + return True + async def _push_file( client: httpx.AsyncClient, project: str, local_root: Path, - source_rel: str, - dest_rel: str, + transfer: _Transfer, ) -> None: """Upload one local file to the destination path in the cloud project.""" - source = _safe_local_path(local_root, source_rel) + source = _safe_local_path(local_root, transfer.source_rel) + _refuse_symlink(source, transfer.source_rel) stat = source.stat() await upload_file( client, project, - dest_rel, + transfer.dest_rel, content=source.read_bytes(), mtime=int(stat.st_mtime), ) @@ -358,10 +487,39 @@ def _safe_local_path(local_root: Path, rel_path: str) -> Path: 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}") - return local_root.joinpath(*candidate.parts) + + 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_webdav_client.py b/tests/cli/cloud/test_webdav_client.py index 15861f23e..c87940953 100644 --- a/tests/cli/cloud/test_webdav_client.py +++ b/tests/cli/cloud/test_webdav_client.py @@ -383,3 +383,44 @@ async def handler(request: httpx.Request) -> httpx.Response: 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"] diff --git a/tests/cli/cloud/test_webdav_transfer.py b/tests/cli/cloud/test_webdav_transfer.py index b6d55b6c7..b40522281 100644 --- a/tests/cli/cloud/test_webdav_transfer.py +++ b/tests/cli/cloud/test_webdav_transfer.py @@ -61,6 +61,24 @@ def _plain(text: str) -> str: 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(): @@ -329,6 +347,8 @@ async def test_push_uploads_new_files_with_their_local_mtime(config_home, tmp_pa 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) @@ -353,6 +373,8 @@ async def test_push_keep_local_overwrites_the_conflicting_cloud_file(config_home 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) @@ -379,6 +401,8 @@ async def test_push_keep_cloud_leaves_the_conflicting_cloud_file_alone(config_ho 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) @@ -406,6 +430,8 @@ async def test_push_keep_both_uploads_the_incoming_copy_under_a_conflict_name( 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) @@ -557,3 +583,295 @@ async def handler(request: httpx.Request) -> httpx.Response: 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: # pragma: no cover + raise AssertionError("must not download over a note that appeared") + + 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" + 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") + + plan = TransferPlan(new=["notes/a#draft.md", "notes/b?v2.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/b?v2.md", + "/webdav/research/notes/c d.md", + ] + assert (root / "notes" / "a#draft.md").read_bytes() == b"body" + assert (root / "notes" / "b?v2.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"] From 97c0612d433951026b54ae637974e11494a5737b Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Mon, 17 Aug 2026 20:24:33 -0500 Subject: [PATCH 3/3] fix(cli): make create-only WebDAV transfers atomic at the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both create-only paths were relying on a check that happened before the write, which is never the same thing as a write that refuses. Pull created and closed an empty destination file, then downloaded, then published with an unconditional `os.replace`. That placeholder was visible for the whole download window — an editor could open it and write real content, and the replace discarded it — and any failure after the download left it behind as a zero-byte note. Nothing is now created at the destination until the bytes exist: the download lands in a sibling temp file and is published with `os.link`, which is atomic and fails outright if the name is taken. A filesystem with no hardlinks (exFAT, some virtual and network mounts) falls back to an exclusive create, which claims the name just as atomically and is weaker only in that a reader can catch the new file mid-write. Push re-listed the cloud once, then issued sequential unconditional PUTs, so the window for the Nth file grew with every file ahead of it in the queue. Create-only uploads now send `If-None-Match: *` and treat 412 as "appeared; left untouched", reported exactly as the pull side reports it. The re-list stays, but as an optimization and a reporting aid rather than the correctness boundary. Explicit keep-local/keep-cloud resolutions deliberately send no precondition — they are instructions to replace. Also drops `?` from the filesystem-touching delimiter test. Windows rejects it as a filename character, which broke the Windows unit job; `?` stays covered at the client level, where nothing is written to disk. This depends on the cloud companion honoring `If-None-Match: *` on PUT (201/204 on create, 412 when the resource exists). Refs #1262 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Drew Cain --- src/basic_memory/cli/commands/cloud/webdav.py | 31 ++- .../cli/commands/cloud/webdav_transfer.py | 161 ++++++++---- tests/cli/cloud/test_webdav_client.py | 44 ++++ tests/cli/cloud/test_webdav_transfer.py | 240 +++++++++++++++++- 4 files changed, 416 insertions(+), 60 deletions(-) diff --git a/src/basic_memory/cli/commands/cloud/webdav.py b/src/basic_memory/cli/commands/cloud/webdav.py index baf0533fc..e6642107f 100644 --- a/src/basic_memory/cli/commands/cloud/webdav.py +++ b/src/basic_memory/cli/commands/cloud/webdav.py @@ -194,26 +194,43 @@ async def upload_file( *, content: bytes, mtime: int, -) -> None: + 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. + 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={"X-OC-Mtime": str(mtime)}, - ) + 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 --- diff --git a/src/basic_memory/cli/commands/cloud/webdav_transfer.py b/src/basic_memory/cli/commands/cloud/webdav_transfer.py index fe5ee8c31..0c824f1c3 100644 --- a/src/basic_memory/cli/commands/cloud/webdav_transfer.py +++ b/src/basic_memory/cli/commands/cloud/webdav_transfer.py @@ -15,7 +15,8 @@ - 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 + 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 @@ -46,6 +47,7 @@ strategy_overwrites_dest, ) from basic_memory.cli.commands.cloud.webdav import ( + DownloadedFile, RemoteFile, WebdavError, download_file, @@ -199,11 +201,11 @@ async def webdav_project_transfer( console.print(f" {transfer.describe()}") if direction == "pull": written = await _pull_file(client, project, local_root, transfer) - if not written: - appeared.append(transfer.dest_rel) - continue else: - await _push_file(client, project, local_root, transfer) + 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]") @@ -352,8 +354,15 @@ def _file_content_hash(path: Path) -> str: # 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. These two -# guards give the WebDAV path the same property, per direction. +# `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( @@ -361,12 +370,12 @@ async def _drop_appeared_on_cloud( ) -> tuple[list[_Transfer], list[str]]: """Re-read the cloud and drop create-only pushes whose path now exists. - This is the analogue of rclone re-listing the destination at copy time. It - narrows the window rather than closing it: nothing here is atomic with the - PUT that follows, so a file created in the remaining moments is still - overwritten. Closing it needs a conditional create on the service side; the - write endpoint does not act on conditional request headers today, so sending - one would be a guard that silently does nothing. + 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: @@ -410,36 +419,33 @@ async def _pull_file( ) -> bool: """Download one cloud file and land it under the destination path. - Returns False when a create-only transfer found the path already taken, so - the caller can report it instead of replacing a note it never compared. + 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) - target.parent.mkdir(parents=True, exist_ok=True) - - # Trigger: the plan cleared this path for creation only. - # Why: an exclusive create is the one check the filesystem can make - # atomically — a stat-then-write would leave the same race open. It also - # refuses a symlink outright, so a link that appeared here is treated as the - # path being taken rather than followed. - # Outcome: claim the name up front, or leave the existing note alone. - if transfer.create_only: - try: - os.close(os.open(target, os.O_CREAT | os.O_EXCL | os.O_WRONLY)) - except FileExistsError: - return False - else: + 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: - downloaded = await download_file(client, project, transfer.source_rel) - except WebdavError: - # Do not leave the empty placeholder behind as a phantom note. if transfer.create_only: - target.unlink(missing_ok=True) - raise + 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) + - # Write to a sibling temp file and rename into place: an interrupted pull - # must never leave a half-written note in the user's knowledge base. +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" ) @@ -447,36 +453,95 @@ async def _pull_file( try: with os.fdopen(handle, "wb") as stream: stream.write(downloaded.content) - # 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 not None: - stamp = downloaded.modified.timestamp() - os.utime(temp_path, (stamp, stamp)) - os.replace(temp_path, target) - finally: - # A no-op once the rename succeeded; the cleanup path when it did not. + # 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, -) -> None: - """Upload one local file to the destination path in the cloud project.""" +) -> 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() - await upload_file( + return await upload_file( client, project, transfer.dest_rel, content=source.read_bytes(), mtime=int(stat.st_mtime), + create_only=transfer.create_only, ) diff --git a/tests/cli/cloud/test_webdav_client.py b/tests/cli/cloud/test_webdav_client.py index c87940953..ae698bc83 100644 --- a/tests/cli/cloud/test_webdav_client.py +++ b/tests/cli/cloud/test_webdav_client.py @@ -424,3 +424,47 @@ async def handler(request: httpx.Request) -> httpx.Response: 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 index b40522281..0e5450a35 100644 --- a/tests/cli/cloud/test_webdav_transfer.py +++ b/tests/cli/cloud/test_webdav_transfer.py @@ -5,7 +5,9 @@ the Personal (rclone) path. """ +import errno import hashlib +import importlib import os import re from contextlib import asynccontextmanager @@ -698,8 +700,10 @@ async def test_pull_leaves_a_local_note_that_appeared_after_planning(config_home # 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: # pragma: no cover - raise AssertionError("must not download over a note that appeared") + 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", @@ -711,6 +715,8 @@ async def handler(request: httpx.Request) -> httpx.Response: # pragma: no cover ) 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 @@ -834,7 +840,9 @@ async def handler(request: httpx.Request) -> httpx.Response: requested.append(request.url.path) return httpx.Response(200, content=b"body") - plan = TransferPlan(new=["notes/a#draft.md", "notes/b?v2.md", "notes/c d.md"]) + # `?` 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, @@ -846,11 +854,10 @@ async def handler(request: httpx.Request) -> httpx.Response: assert requested == [ "/webdav/research/notes/a#draft.md", - "/webdav/research/notes/b?v2.md", "/webdav/research/notes/c d.md", ] assert (root / "notes" / "a#draft.md").read_bytes() == b"body" - assert (root / "notes" / "b?v2.md").read_bytes() == b"body" + assert (root / "notes" / "c d.md").read_bytes() == b"body" @pytest.mark.asyncio @@ -875,3 +882,226 @@ async def handler(request: httpx.Request) -> httpx.Response: ) 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", "*")]