fix(cli): run Team push/pull over WebDAV instead of storage credentials

[groksrc]

Fixes the Team-workspace half of #1262.

Problem

bm cloud pull / bm cloud push are documented as the Team-safe sync path, and their
transfer logic is deliberately not role-gated — but on an organization workspace they
could not run at all. Both require a tenant-scoped rclone remote, provisioning one mints
object-storage credentials, and that endpoint is restricted to workspace owners. Every
other member was pointed at bm cloud setup and got a 403 from it.

Widening that endpoint would be the wrong fix. Storage credentials are scoped to an
entire tenant bucket, so they cannot express "this member may read project A but not
project B" — they would grant more access than the service itself does.

Approach

Team transfers now run over the service's WebDAV surface, where every request is
authorized against the caller's access to the specific project (GET/PROPFIND need
viewer, PUT needs editor). bm cloud setup drops out of the Team path entirely: no
remote to provision, no credentials to mint, no 403.

The Personal rclone path is untouched, and rclone stays an internal detail of it.

Design choices

• webdav.py, a sibling of upload.py, rather than growing upload.py.
  upload.py is the implementation of one command — a directory walk that prints its
  own progress and owns that command's filtering rules. The new module is protocol only
  (PROPFIND, GET, PUT, entity-tag normalization), with no CLI output and no policy,
  so the upload command and the push/pull engine can share one definition of how a
  project's files are addressed. upload.py's only change is to build its remote path
  through that shared helper.
• transfer.py holds the plan vocabulary (TransferPlan, TransferDirection,
  ConflictStrategy, and the two conflict-resolution helpers), moved out of
  rclone_commands.py. It is transport-agnostic, and keeping it separate means the
  WebDAV engine does not import rclone's subprocess machinery to reuse it.
• PROPFIND is walked, not requested recursively. The surface answers for one
  collection at a time, so a project listing is a breadth-first walk; each path is
  listed once. Entry names come from displayname (the literal, unencoded name), with
  the percent-decoded final href segment as the fallback for servers that omit it.
• Comparison is entity tag plus size, with a documented fallback. Size settles it
  whenever it differs, so a large file is never hashed to learn what its length already
  proved. A single-part entity tag is an MD5 of the bytes and is compared directly. A
  tag that cannot be a content hash — absent, opaque, weak (W/), or multipart-shaped
  (-N) — falls back to last-modified plus size. When neither a usable tag nor a
  timestamp is available and sizes match, the file is reported as uncomparable and the
  transfer aborts rather than guessing. Treating such a file as identical would lose an
  edit; treating it as always-conflicting would block every transfer.
• Pull carries the cloud's timestamp onto the local file, the way rclone preserves
  modtimes, which is what keeps the timestamp fallback meaningful after a pull. Files are
  written to a sibling temp file and renamed, so an interrupted pull cannot leave a
  half-written note behind.
• Paths from the listing are validated before use. An absolute path, a .. segment,
  or a backslash is refused at planning time — remote input decides where we write on
  pull, so it is checked before a plan is even presented.
• Ignore patterns are applied to the cloud listing as well as the local scan, the same
  way rclone's --filter-from applies to both sides of a transfer.

Unchanged

Same commands, same flags, same defaults. --on-conflict [fail|keep-local|keep-cloud|keep-both]
still defaults to fail. Transfers stay additive — nothing is ever deleted on the
destination — and deletions are still not propagated. No flags added, removed, or renamed.
bisync remains gated to Personal workspaces and is untouched.

Review follow-ups

Three findings from review, all fixed in the second commit.

• Symlinks. The path guard was lexical, and a lexical check 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 outside local_root. The local scan now
  skips symlinked files and does not descend into symlinked directories, matching the local
  project scanner; containment is enforced on the resolved parent chain; and a link at the
  final component is refused where a transfer would follow it.
• Percent-encoding. ? and # are structural URL delimiters, not path data, so a note
  named a#draft.md was requested as a. Request paths are now encoded per segment with
  the separators preserved, and the self-href comparison decodes both sides.
• Stale plan. A path classified as new was transferred unconditionally, so a file
  that appeared on the destination between planning and writing was destroyed — even under
  the default --on-conflict fail. See below.

On the stale-plan window

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, not against the earlier rclone check.

A first pass gave this transport a listing-based equivalent. That was not enough — a check
that happens before the write is never the same thing as a write that refuses — so the
guarantee now lives at the write itself, in both directions:

• Pull downloads to a sibling temp file and publishes with os.link, which is atomic
  and fails outright if the name is taken. Nothing exists at the destination until the bytes
  do, so there is no placeholder for an editor to fill and have discarded, and no failure
  path that leaves a zero-byte note behind. On a filesystem without hardlinks (exFAT, some
  virtual and network mounts — the kind this project already accommodates elsewhere), it
  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. A real failure still surfaces
  rather than being swallowed.
• Push sends If-None-Match: * on create-only uploads and treats 412 as "appeared;
  left untouched". The pre-transfer re-list stays, but as an optimization and a reporting
  aid: it avoids uploading bytes certain to be refused and names collisions up front. It is
  explicitly not the correctness boundary, because its window grows with every file ahead
  of the Nth in the queue.

Either way the file is left alone and named in the output, so a re-run compares it and
--on-conflict applies. Explicit keep-local / keep-cloud resolutions deliberately send
no precondition and still replace — those are instructions, not stale guesses.

Dependency — please land the server changes first

This codes against companion cloud-side changes on two fronts. Neither is deployed yet.

1. getetag and getlastmodified on the PROPFIND response, and ETag / Last-Modified /
   Content-Length headers on GET.
2. If-None-Match: * honored on PUT: create normally (201/204), and 412 when the
   resource already exists. Push's no-clobber guarantee rests on this; until it lands, a
   create-only PUT is unconditional, which is the behavior this PR is fixing. Until it is,
   every file present on both sides will be reported as uncomparable and the transfer will
   abort — which is the intended fail-fast behavior, not a silent wrong answer, but it does
   mean this is not usable end to end until the server side lands.

Accordingly, the tests here use fixture PROPFIND XML and mocked HTTP responses. No live
end-to-end call was made against production cloud, deliberately: it would not yet return
validators, so a failure there would prove nothing.

The companion server now paginates each PROPFIND collection to completion and fails
closed if object storage reports truncation without a continuation token. Planning
therefore never proceeds from a partial listing.

Tests

New:

• tests/cli/cloud/test_webdav_client.py — PROPFIND parsing and the directory walk
  (including a child collection whose href collides with the collection being listed),
  name resolution and its href fallback, entity-tag normalization and the single-part /
  multipart / weak / opaque cases, GET and PUT, and the error mapping for rejected and
  transport-level failures.
• tests/cli/cloud/test_webdav_transfer.py — plan classification, ignore filtering on
  both sides, the path-escape refusal, the etag-unavailable fallback (match, diverged
  timestamp, sub-second drift, and the no-validator-at-all case), and all four
  --on-conflict strategies for both push and pull, plus dry-run and verbose.
• tests/cli/cloud/test_project_sync_command.py — Team routing at the CLI level: both
  directions go over WebDAV and never reach rclone, storage credentials, or the
  bm cloud setup hint; the conflict gate, the uncomparable-file abort, the strategy and
  dry-run/verbose pass-through, and the failure and missing-project paths.

Added for the review follow-ups: symlinked files and directories excluded from the scan;
pull and push both refusing to transfer through a symlinked directory; keep-cloud refusing
to overwrite a symlinked note; a local and a cloud file appearing after planning being left
untouched and reported; explicit resolutions still overwriting; no placeholder left behind
when a download fails; and #, ?, and space in filenames round-tripping through list,
download, and upload.

Added for the atomicity round: a note created during the download surviving; content
written into the destination during the download surviving, with an assertion that nothing
is staged there beforehand; a publication failure after a successful download leaving nothing
behind; the no-hardlink path still refusing to clobber; If-None-Match: * sent on create-only
pushes and 412 reported as appeared; conflict copies being conditional too; and keep-local
deliberately sending no precondition.

One Windows-only fix: ? is not a legal filename character there, so the delimiter test that
actually writes to disk now covers # and space, and ? stays covered at the client level
where nothing touches the filesystem. That was a genuine CI failure on the previous push, not
a pre-existing one.

Commands run:

uv run pytest tests/cli/cloud/ tests/test_rclone_commands.py tests/cli/test_upload.py
  -> 255 passed, 2 failed
uv run ruff check src tests        -> passed
uv run ruff format --check src tests -> passed
uv run ty check src tests test-int -> 4 diagnostics

The 2 test failures and all 4 type diagnostics are pre-existing and reproduce identically
on main at ea38fd76:

• test_cloud_pull_clean_transfers and test_cloud_prune_dry_run_previews_without_deleting
  assert on [dim]-styled console output, which carries ANSI escapes in a local terminal.
  New tests added here strip styling before asserting, so they are not affected. Running
  that file with color enabled surfaces 7 failures for the same reason; the sorted failure
  list is byte-identical between this branch and main.
• The type diagnostics are all unresolved-import for pymilvus in
  tests/repository/test_milvus_repository.py, an optional dependency not installed in
  this environment.

Not run: the full suite, the Postgres matrix, and any live cloud call.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85845272a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Revalidate destinations before transferring planned files

When another Team member creates a cloud file—or a local process creates a local file—after webdav_project_diff() classifies the path as new, these unconditional GET/PUT calls transfer against the stale plan. On pull, _pull_file() ultimately uses os.replace() and destroys the newly created local note; on push, the unconditional PUT can overwrite the teammate's new cloud note, even with the default --on-conflict fail. Use conditional destination creation or revalidate the planned validator immediately before writing so concurrent creations abort rather than overwrite canonical bytes.

AGENTS.md reference: AGENTS.md:L156-L160

Useful? React with 👍 / 👎.

[groksrc]

Fixed in 0408031.

The first thing I checked was whether the Personal path has the same window, since if it did, Team would not be worse and a narrower fix plus a documented residual would have been the right call. It does not — 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. This transport had no equivalent, which made it a real regression relative to Personal rather than a shared limitation.

So create-only transfers now carry that classification forward (_Transfer.create_only) and get the same property, per direction:

• Pull claims the path with os.open(..., O_CREAT | O_EXCL). That is atomic, so there is no residual window here, and it refuses a symlink outright as a bonus — a link that appeared at the path is treated as the path being taken rather than followed.
• Push re-reads the cloud immediately before the transfer loop and drops any create-only path that now exists. This narrows the window rather than closing it: nothing there is atomic with the PUT that follows.

Either way the file is left alone and named in the output, so a re-run compares it and --on-conflict applies. An explicit keep-local / keep-cloud resolution still overwrites — that is an instruction, not a stale guess — and there are tests for both halves.

On If-None-Match: *: I checked the write endpoint before relying on it, and it does not act on conditional request headers at all — it reads the request only to size and store the body. Sending the header would have been a guard that silently does nothing, so I did not ship it. Closing the push window properly needs a server-side conditional create (If-None-Match: * → 412 when the resource exists). That is out of scope here; it is called out in the PR body for a separate issue.

Tests: a local note and a cloud note each appearing after planning are left untouched and reported; both explicit resolutions still overwrite; and a failed download leaves no empty placeholder behind from the exclusive create.

[chatgpt-codex-connector]

Reject symlinks that escape the project root

When a project contains a symlinked file or directory, this lexical check still accepts the path: scan_local_files() and _push_file() follow file symlinks and can upload data from outside the project to the Team workspace, while _pull_file() follows a symlinked parent and writes the downloaded file outside local_root. The normal project index explicitly skips symlinked files in src/basic_memory/index/local_project.py:236-272; resolve paths and enforce containment here as well, and skip symlinked local files during scanning.

AGENTS.md reference: AGENTS.md:L156-L160

Useful? React with 👍 / 👎.

[groksrc]

Correct, and fixed in 0408031. You are right that there was no symlink handling at all — _safe_local_path was purely lexical, and a lexical check cannot see a link.

Following the precedent you pointed at in local_project.py:

• scan_local_files now walks with followlinks=False (explicit rather than relying on the default) and skips symlinked files, so push can never read bytes from outside the project boundary.
• Containment is enforced on the resolved parent chain rather than the lexical path — that is the part that actually decides which directory is read from or written into, so a symlinked parent no longer defeats it.
• The final component is handled separately, because what should happen to a link there depends on what the transfer is about to do to it. _refuse_symlink refuses where the transfer would 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, and treating it as "path taken" and skipping is better than aborting the whole transfer over a file the user may legitimately have symlinked.

Tests cover both directions refusing to transfer through a symlinked directory, symlinked files and directories being excluded from the scan, and keep-cloud refusing to overwrite a symlinked note (asserting the outside file's contents are untouched).

[chatgpt-codex-connector]

Percent-encode WebDAV path segments

When a valid POSIX filename or project name contains ? or #, concatenating it into a raw URL does not let httpx encode it as path data: ? starts a query and # starts a fragment. A listed notes/a#draft.md, for example, is subsequently requested as notes/a, causing a 404 or transferring the wrong object's bytes. Encode each project/path segment while preserving the intended / separators before passing the URL to httpx.

AGENTS.md reference: AGENTS.md:L156-L160

Useful? React with 👍 / 👎.

[groksrc]

Correct, and fixed in 0408031. My docstring asserted something that is not true: httpx percent-encodes characters that are unsafe within a component, but ? and # are structural delimiters, so it reads them as the start of a query and a fragment. A note named a#draft.md was requested as a.

webdav_path now encodes the project name and each path segment while preserving / as the separator (quote(rel_path, safe="/")). One knock-on: _same_path compares a PROPFIND href against the request path, and the request path is now encoded while the href may not be, so both sides are decoded before comparison — the question is which resource is named, not how it was spelled on the wire.

Tests: a parametrized check on webdav_path for #, ?, space, a space in the project name, and nested separators staying separators; a round trip through list → download for a#draft.md; and transfer-level tests that pull and push #, ?, and space filenames and assert on the paths the server actually receives.

[groksrc]

Review follow-up: two create-only transfer paths still need hard atomicity before this is safe to merge.

1. Push: _drop_appeared_on_cloud re-lists once before a sequential series of unconditional PUTs. A teammate can create a later path after that listing but before its PUT, and the default --on-conflict fail path will overwrite it. The cloud companion should honor If-None-Match: *; create-only uploads here should send it and treat 412 as “appeared; left untouched.” The existing re-list is useful for reporting/efficiency, but it cannot be the correctness boundary.

2. Pull: _pull_file creates and closes an empty destination placeholder before the network download, then publishes with unconditional os.replace. If temp-file creation, writing, timestamping, or replacement fails after the download, the empty placeholder remains. An editor can also modify or replace that visible placeholder while the download is in flight, and os.replace destroys the new content. Please download to a sibling temp file first, then publish with an atomic no-replace operation; if the destination exists at publication time, discard the temp file and report it as appeared. Add regression coverage for a destination created/modified during download and for a post-download publication failure.

The symlink and percent-encoding follow-ups otherwise look well addressed.

[groksrc]

Both fixed in 97c0612. You were right that the O_EXCL change claimed the name but not the content — and the pull restructure below is a bigger change than a placement tweak, because the ordering was wrong, not just the timing.

Pull. Nothing is created at the destination until the bytes exist. The download lands in a sibling temp file, and publication is os.link, which is atomic and fails outright when the name is taken. Both defects you named are gone: there is no placeholder for an editor to fill and have discarded, and no failure path that leaves a zero-byte note (the only thing created before the download is the parent directory).

On hardlink availability — you asked me to evaluate and say what I chose. I kept os.link as the publish, because it is the only portable atomic no-replace operation and it additionally guarantees no reader ever sees a half-written file. But this project already accommodates virtual mounts explicitly (there is a --local-no-preallocate flag in the rclone path specifically for Google Drive File Stream), and exFAT and several network mounts have no hardlinks at all, so failing outright there would break pull for those users. So an OSError that is not FileExistsError falls through to an exclusive create, which claims the name just as atomically. It is weaker only in that a reader can catch the new file mid-write — no unlinked filesystem can do better — and it is not a stats-then-writes fallback. A real failure (no space, no permission) still surfaces from the create rather than being swallowed.

One consequence worth flagging: the download now runs before we discover the name is taken, so a raced pull wastes one download. That is the direct cost of "nothing at the destination before the bytes exist," and it seemed clearly the right trade.

Push. Create-only uploads now send If-None-Match: * and treat 412 as "appeared; left untouched", reported through the same path as pull. The re-list stays, but its role is now stated as what it actually is — an optimization and a reporting aid, explicitly not the correctness boundary, since its window grows with every file ahead of the Nth in the queue. Explicit keep-local / keep-cloud deliberately send no precondition; those are instructions to replace, and there is a test asserting the header is absent for them. Conflict copies under keep-both are conditional too — they are creates.

I did not need to stop on the backend question: s3_file_service.delete_file_if_unchanged already drives a storage-native If-Match precondition and handles 412, with a comment saying the precondition "is what actually closes the window" for exactly this class of race. So conditional create is feasible on this backend rather than something the storage layer would have to fake. The dependency is stated in the PR body alongside the validators one.

Also in this push: a genuine CI failure I introduced last round. ? is not a legal filename character on Windows, so the delimiter test that actually writes to disk broke the Windows unit job. That test now covers # and space; ? stays covered at the client level, where nothing touches the filesystem.

Tests: uv run pytest tests/cli/cloud/ tests/test_rclone_commands.py tests/cli/test_upload.py → 255 passed, 2 failed (both pre-existing). New coverage for a note created during the download, content written into the destination during the download (asserting nothing is staged there beforehand), a post-download publication failure leaving nothing behind, the no-hardlink path still refusing to clobber, the 412 path, and the header being present or absent per strategy. ruff check / format --check clean; ty check src tests test-int → the same 4 pre-existing pymilvus diagnostics. The pre-existing failures in test_project_sync_command.py are still exactly 7 and diff-identical to main.

[groksrc]

Both addressed in 97c0612.

Pull — nothing is created at the destination until the bytes exist. The empty placeholder is gone. The download now lands in a sibling temp file, fully written and timestamped, and only then is the name claimed. The timestamp is applied before publication deliberately: mtime lives on the inode, so a hardlinked publish shares it and there is no window in which the published note carries the wrong one.

Publication is os.link, which is atomic and fails outright when the name is taken — a note that appeared during the download is never destroyed, and no reader observes a half-written file. FileExistsError returns the same "appeared; left untouched" result as before.

Filesystems that cannot hardlink (exFAT, some virtual and network mounts) fall back to an exclusive create, which claims the name just as atomically. That fallback is weaker in exactly one way, stated rather than hidden: a reader can catch the new file mid-write. No unlinked filesystem can do better. Genuine failures such as no space or no permission still surface rather than being swallowed by the fallback.

An explicit --on-conflict keep-cloud still uses os.replace, since that is an instruction to replace what is there.

Push — the conditional header is now the correctness boundary. Create-only uploads send If-None-Match: * and treat 412 as "appeared; left untouched". The pre-push re-list is kept for reporting and to avoid pointless round trips, but it is no longer what stands between a teammate and a lost note. keep-local deliberately does not send the header — it is an explicit instruction to overwrite — while keep-both conflict copies do, since those are new names.

Coverage for a destination created during the download, content written into the destination during the download, a publication failure after a successful download leaving nothing behind, the no-hardlink fallback, and the conditional-push paths including both strategy distinctions above.

200 tests pass. The 7 failures in test_project_sync_command.py are pre-existing — I ran the file on main and on this branch and diffed the sorted failure lists, which are identical.

Still depends on the companion server change, which now covers both the validators and If-None-Match support.