Cordy/cairn-desktop
1
0
Fork 0
Code Issues 25 Pull requests Projects Releases Packages Wiki Activity Actions

P2-3: GET, PUT, MKCOL, DELETE, MOVE — complete the Remote interface #23

New issue
Closed
opened 2026-09-10 17:41:14 +00:00 by Cordy · 2 comments
Cordy commented 2026-09-10 17:41:14 +00:00
Owner
Copy link

Depends on P2-2. Completes remote.Remote against a real Cairn for everything except resumable upload (P2-4).

Files

  • Create: internal/remote/webdav.go, internal/remote/webdav_test.go

Verb mapping

Interface method HTTP Notes
Get(path) GET Support a Range header for resumed downloads
Put(path, r, mod) PUT Simple upload; large files go via tus in P2-4
Mkcol(path) MKCOL 405 Method Not Allowed means it already exists — treat as success
Delete(path) DELETE 404 means already gone — treat as success
Move(from, to) MOVE Requires a Destination: header with a full absolute URL, and Overwrite: T

Three things that will bite

  1. MOVE needs an absolute Destination URL, percent-encoded, not a relative path. Getting this wrong yields a confusing 400.
  2. Put must return the new Entry with its ETag. Some servers return the ETag in the PUT response header; some do not. If absent, follow with a PROPFIND on that path. Do not guess or leave it empty — the engine writes it into state, and an empty ETag means the next pass sees a phantom remote change.
  3. Idempotency: Delete on a missing path and Mkcol on an existing one both mean "the world is already how I want it". Returning an error would make the engine skip a path forever.

Steps

  • Write failing tests against httptest covering: round-trip PUT→GET; ranged GET returns the right slice; MKCOL on an existing collection succeeds; DELETE of a missing path succeeds; MOVE sends an absolute encoded Destination; PUT with no ETag in the response falls back to PROPFIND.
  • Run; confirm failure.
  • Implement.
  • Add var _ Remote = (*Client)(nil) so the compiler proves the real client is substitutable for MemRemote.
  • Run tests; confirm they pass.
  • Commit: git commit -s -m "feat(remote): GET/PUT/MKCOL/DELETE/MOVE over WebDAV"

Acceptance criteria

  • *Client satisfies remote.Remote.
  • Every engine test from phase 1 could, in principle, run against this client unchanged.
  • Idempotent verbs never return an error for an already-correct state.
Depends on P2-2. Completes `remote.Remote` against a real Cairn for everything except resumable upload (P2-4). ## Files - Create: `internal/remote/webdav.go`, `internal/remote/webdav_test.go` ## Verb mapping | Interface method | HTTP | Notes | |---|---|---| | `Get(path)` | `GET` | Support a `Range` header for resumed downloads | | `Put(path, r, mod)` | `PUT` | Simple upload; large files go via tus in P2-4 | | `Mkcol(path)` | `MKCOL` | `405 Method Not Allowed` means it already exists — treat as success | | `Delete(path)` | `DELETE` | `404` means already gone — treat as success | | `Move(from, to)` | `MOVE` | Requires a `Destination:` header with a **full absolute URL**, and `Overwrite: T` | ## Three things that will bite 1. **`MOVE` needs an absolute `Destination` URL**, percent-encoded, not a relative path. Getting this wrong yields a confusing 400. 2. **`Put` must return the new `Entry` with its ETag.** Some servers return the ETag in the `PUT` response header; some do not. If absent, follow with a `PROPFIND` on that path. Do not guess or leave it empty — the engine writes it into state, and an empty ETag means the next pass sees a phantom remote change. 3. **Idempotency:** `Delete` on a missing path and `Mkcol` on an existing one both mean "the world is already how I want it". Returning an error would make the engine skip a path forever. ## Steps - [ ] Write failing tests against `httptest` covering: round-trip PUT→GET; ranged GET returns the right slice; MKCOL on an existing collection succeeds; DELETE of a missing path succeeds; MOVE sends an absolute encoded `Destination`; PUT with no ETag in the response falls back to PROPFIND. - [ ] Run; confirm failure. - [ ] Implement. - [ ] **Add `var _ Remote = (*Client)(nil)`** so the compiler proves the real client is substitutable for `MemRemote`. - [ ] Run tests; confirm they pass. - [ ] Commit: `git commit -s -m "feat(remote): GET/PUT/MKCOL/DELETE/MOVE over WebDAV"` ## Acceptance criteria - `*Client` satisfies `remote.Remote`. - Every engine test from phase 1 could, in principle, run against this client unchanged. - Idempotent verbs never return an error for an already-correct state.
Cordy commented 2026-09-11 01:50:21 +00:00
Author
Owner
Copy link

Amendment — 2026-09-11: phase-1 hand-off (binding rulings Task 3 F5, Task 12 CV4; final review X2)

Task 3 F5 — address existing entries by their native name

GET, PUT, DELETE and MOVE on an existing entry use the native name #22 observed in
the listing, not a URL built from the NFC engine path. A byte-preserving server can store NFD or
other non-canonical bytes, and a rebuilt URL then returns 404. Under R2 that reads as "already
gone", which is one step from a deletion. Percent-encoding the engine path is only for names the
client itself creates. See the #22 amendment for the full ruling.

Task 12 CV4 — the MOVE semantics the engine relies on

  • (i) MOVE over an existing file. The engine needs this when a local rename lands on a name
    that is already synced. Decide emits it only when the destination is unchanged against its
    state row, so the overwrite replaces only bytes that are already synced. Keep Overwrite: T
    as specified above; remote.go documents file-onto-file overwrite. Satisfied, no change.

  • (ii) ETag stability across MOVE. After the Task 12 F3 fix, the engine records the prior
    row's metadata after a move, not a post-move Stat. Correctness therefore no longer depends on
    whether the server keeps the ETag, and an unstable ETag costs one re-download per rename.
    #29's integration run against cairnd records which behaviour cairnd has.

  • (iii) Missing parent collections. Binding:

    • Client.Move creates every missing ancestor collection of to before it sends MOVE. It
      uses MKCOL, where 405 means the collection already exists, as Put does.
    • Client.Move maps a 404 source to fs.ErrNotExist (R2).
    • Add the httptest case "MOVE into a missing parent collection succeeds".
    • Add "creating any missing parent collections" to the Remote.Move doc comment, so that
      MemRemote and Client share one contract.

    RFC 4918 §9.9.4 has the server answer 409 for a missing intermediate collection. Without the
    MKCOL chain, every rename into a new folder becomes a permanent pair of Skips.

Final review X2 — conditional requests close the race on the server

The engine now re-reads each copy just before it replaces or removes it. It compares a local
copy's size, mtime and FileID, and a server copy's presence and ETag, with the scan that decided
the operation. On a mismatch it skips that operation and changes nothing. This is commit
699ec0a (fix(sync): re-check a copy against the scan before replacing or removing it).

That shrinks the window to the moment between the re-read and the request, but does not close
it. Close it on the server:

  • PUT over an existing object, DELETE and MOVE (source) send
    If-Match: "<the ETag the scan saw>".
  • A PUT or MOVE to a path the scan saw as absent sends If-None-Match: * (for PUT) or
    Overwrite: F (for MOVE).
  • A 412 Precondition Failed maps to a distinct error that the engine turns into the same
    "changed while syncing" Skip. It must not be a failure, and must not be retried as though
    unconditional.

The engine has to pass the expected ETag in. Decide the shape here: conditional variants or an
options argument on Remote. Implement the same semantics in MemRemote so the phase-1 engine
tests keep driving it. This is an engine change as well as a client change.

## Amendment — 2026-09-11: phase-1 hand-off (binding rulings Task 3 F5, Task 12 CV4; final review X2) ### Task 3 F5 — address existing entries by their native name `GET`, `PUT`, `DELETE` and `MOVE` on an **existing** entry use the native name #22 observed in the listing, not a URL built from the NFC engine path. A byte-preserving server can store NFD or other non-canonical bytes, and a rebuilt URL then returns 404. Under R2 that reads as "already gone", which is one step from a deletion. Percent-encoding the engine path is only for names the client itself creates. See the #22 amendment for the full ruling. ### Task 12 CV4 — the MOVE semantics the engine relies on - **(i) MOVE over an existing file.** The engine needs this when a local rename lands on a name that is already synced. `Decide` emits it only when the destination is unchanged against its state row, so the overwrite replaces only bytes that are already synced. Keep `Overwrite: T` as specified above; `remote.go` documents file-onto-file overwrite. Satisfied, no change. - **(ii) ETag stability across MOVE.** After the Task 12 F3 fix, the engine records the prior row's metadata after a move, not a post-move Stat. Correctness therefore no longer depends on whether the server keeps the ETag, and an unstable ETag costs one re-download per rename. #29's integration run against `cairnd` records which behaviour `cairnd` has. - **(iii) Missing parent collections.** Binding: - `Client.Move` creates every missing ancestor collection of `to` before it sends `MOVE`. It uses `MKCOL`, where `405` means the collection already exists, as `Put` does. - `Client.Move` maps a `404` source to `fs.ErrNotExist` (R2). - Add the httptest case **"MOVE into a missing parent collection succeeds"**. - Add "creating any missing parent collections" to the `Remote.Move` doc comment, so that `MemRemote` and `Client` share one contract. RFC 4918 §9.9.4 has the server answer 409 for a missing intermediate collection. Without the MKCOL chain, every rename into a new folder becomes a permanent pair of Skips. ### Final review X2 — conditional requests close the race on the server The engine now re-reads each copy just before it replaces or removes it. It compares a local copy's size, mtime and FileID, and a server copy's presence and ETag, with the scan that decided the operation. On a mismatch it skips that operation and changes nothing. This is commit 699ec0a (`fix(sync): re-check a copy against the scan before replacing or removing it`). That shrinks the window to the moment between the re-read and the request, but does not close it. Close it on the server: - `PUT` over an existing object, `DELETE` and `MOVE` (source) send `If-Match: "<the ETag the scan saw>"`. - A `PUT` or `MOVE` to a path the scan saw as absent sends `If-None-Match: *` (for `PUT`) or `Overwrite: F` (for `MOVE`). - A `412 Precondition Failed` maps to a distinct error that the engine turns into the same "changed while syncing" Skip. It must not be a failure, and must not be retried as though unconditional. The engine has to pass the expected ETag in. Decide the shape here: conditional variants or an options argument on `Remote`. Implement the same semantics in `MemRemote` so the phase-1 engine tests keep driving it. This is an engine change as well as a client change.
Cordy commented 2026-09-11 05:11:54 +00:00
Author
Owner
Copy link

Done

What was built

  • Client.Get/GetFrom/Put/Mkcol/Delete/Move over WebDAV, with var _ Remote = (*Client)(nil).
  • Precondition{ETag, Absent} on Put/Delete/Move (amendment X2), the same semantics in MemRemote, ErrPreconditionFailed on 412.
  • The engine sends preconditions on upload, delete-remote, move and the post-move PUT; a refused write is a "changed while syncing" Skip, never retried.
  • MKCOL parent-chain creation before PUT/MOVE (cairnd 404s a write into a missing parent), PROPFIND fallback when PUT returns no ETag, %-name refusal, native-name addressing (F5/F7).
  • Move guards cairnd's destructive Overwrite: T (self-move, subtree move, a collection on either side).

Tests

  • 15 new Client tests plus TestEngineSyncsThroughClient in webdav_test.go; engine conditional-write tests in recheck_test.go; 3 MemRemote precondition tests; 12/12 mutations killed.
  • Live check against a throwaway cairnd bd006ef via cairnd-dev.sh (P2-R18): PUT/GET/MKCOL/DELETE/MOVE round trips, Overwrite: F → 412, If-Match/If-None-Match confirmed ignored.
  • CI run #21, linux/arm64, green — go vet and go test -count=1 ./... all ok, coverage total 92.1%.

Acceptance criteria

  • *Client satisfies remote.Remote — var _ Remote = (*Client)(nil).
  • Every phase-1 engine test can run against Client unchanged — TestEngineSyncsThroughClient, contract parity with MemRemote.
  • Idempotent verbs never error on an already-correct state — MKCOL 405→nil, DELETE 404→fs.ErrNotExist (engine treats as done).
  • The issue's six httptest cases, F5 (native names), CV4(iii) (MKCOL chain before MOVE), X2 (Precondition shape, wire headers, 412 handling) are all met — see the report for test names.

Rulings

  • P2-R1: push to main after clean review; close only when every Actions run for the pushed commit is green; commit subject = issue message, git commit -s, Co-Authored-By.
  • P2-R3: Client.Get/Delete/Move on a missing path give both ErrNotFound and fs.ErrNotExist; MKCOL 405→nil.
  • P2-R4: Remote.Put's ETag contract is one-way; "identical content ⇒ identical ETag" is a MemRemote-only property.
  • P2-R13: the sync root is a WebDAV collection URL (<server>/dav/home/).
  • P2-R15: cairnd deviations tolerated — MKCOL chain, Depth 1/0 always, PROPFIND after PUT, % names refused as a per-file Skip.
  • P2-R16: conditional requests implemented; cairnd honours Overwrite: F but not If-Match — the engine's re-check is the real guard.
  • P2-R18: live checks used cairnd-dev.sh only, never ~/Cairn, 192.168.10.249, or the owner's credentials.
  • Ruling (CV5/review F4): the untested "Overwrite: T whenever !want.Absent" mutation survived every test with no dedicated row; fixed by adding "Move without Absent where nothing is at the destination" to TestClientSendsTheCallersPreconditions.

Deferred

  • Review Minors F1 (PROPFIND fallback can record another client's equal-size write), F2 (contract overstates precondition enforcement on cairnd), F3 (a failed MOVE/PUT can leave new parent collections behind), F5 (weak ETags sent as strong If-Match), F6 (server-notes hand-off on If-None-Match) — deferred to the final review / #26/#27/#29.
  • #21's deferred redirect minor (Go turns a 301/302 on DELETE/MOVE into a GET) — unchanged, still open.

Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI.

**Done** - [feat(remote): GET/PUT/MKCOL/DELETE/MOVE over WebDAV](http://192.168.10.245/Cordy/cairn-desktop/commit/a1263dc6420d367186d84018042c98d8e5ad2c16) - [test(remote): a Move without Absent onto an empty path sends Overwrite: F](http://192.168.10.245/Cordy/cairn-desktop/commit/278ab7f09f40b3f537b0c9d3701086d3065bee31) **What was built** - `Client.Get`/`GetFrom`/`Put`/`Mkcol`/`Delete`/`Move` over WebDAV, with `var _ Remote = (*Client)(nil)`. - `Precondition{ETag, Absent}` on Put/Delete/Move (amendment X2), the same semantics in MemRemote, `ErrPreconditionFailed` on 412. - The engine sends preconditions on upload, delete-remote, move and the post-move PUT; a refused write is a "changed while syncing" Skip, never retried. - MKCOL parent-chain creation before PUT/MOVE (cairnd 404s a write into a missing parent), PROPFIND fallback when PUT returns no ETag, `%`-name refusal, native-name addressing (F5/F7). - Move guards cairnd's destructive `Overwrite: T` (self-move, subtree move, a collection on either side). **Tests** - 15 new Client tests plus `TestEngineSyncsThroughClient` in `webdav_test.go`; engine conditional-write tests in `recheck_test.go`; 3 MemRemote precondition tests; 12/12 mutations killed. - Live check against a throwaway cairnd bd006ef via `cairnd-dev.sh` (P2-R18): PUT/GET/MKCOL/DELETE/MOVE round trips, `Overwrite: F` → 412, If-Match/If-None-Match confirmed ignored. - CI run [#21](http://192.168.10.245/Cordy/cairn-desktop/actions/runs/21), linux/arm64, green — `go vet` and `go test -count=1 ./...` all `ok`, coverage total 92.1%. **Acceptance criteria** - `*Client` satisfies `remote.Remote` — `var _ Remote = (*Client)(nil)`. - Every phase-1 engine test can run against Client unchanged — `TestEngineSyncsThroughClient`, contract parity with MemRemote. - Idempotent verbs never error on an already-correct state — MKCOL 405→nil, DELETE 404→fs.ErrNotExist (engine treats as done). - The issue's six httptest cases, F5 (native names), CV4(iii) (MKCOL chain before MOVE), X2 (Precondition shape, wire headers, 412 handling) are all met — see the report for test names. **Rulings** - P2-R1: push to main after clean review; close only when every Actions run for the pushed commit is green; commit subject = issue message, `git commit -s`, Co-Authored-By. - P2-R3: `Client.Get`/`Delete`/`Move` on a missing path give both `ErrNotFound` and `fs.ErrNotExist`; MKCOL 405→nil. - P2-R4: `Remote.Put`'s ETag contract is one-way; "identical content ⇒ identical ETag" is a MemRemote-only property. - P2-R13: the sync root is a WebDAV collection URL (`<server>/dav/home/`). - P2-R15: cairnd deviations tolerated — MKCOL chain, Depth 1/0 always, PROPFIND after PUT, `%` names refused as a per-file Skip. - P2-R16: conditional requests implemented; cairnd honours `Overwrite: F` but not If-Match — the engine's re-check is the real guard. - P2-R18: live checks used `cairnd-dev.sh` only, never ~/Cairn, 192.168.10.249, or the owner's credentials. - Ruling (CV5/review F4): the untested "`Overwrite: T` whenever `!want.Absent`" mutation survived every test with no dedicated row; fixed by adding "Move without Absent where nothing is at the destination" to `TestClientSendsTheCallersPreconditions`. **Deferred** - Review Minors F1 (PROPFIND fallback can record another client's equal-size write), F2 (contract overstates precondition enforcement on cairnd), F3 (a failed MOVE/PUT can leave new parent collections behind), F5 (weak ETags sent as strong If-Match), F6 (server-notes hand-off on If-None-Match) — deferred to the final review / #26/#27/#29. - #21's deferred redirect minor (Go turns a 301/302 on DELETE/MOVE into a GET) — unchanged, still open. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._
Cordy closed this issue 2026-09-11 05:11:59 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
v0.2.0-integration
v0.1.0-engine
Labels
Clear labels   
data-integrity

Touches correctness where a bug means data loss

  
engine

Sync engine core (internal/sync)

  
platform

OS-specific code, packaging, signing

  
procurement

Manual task — accounts, legal, hardware. Not code.

  
remote

WebDAV / tus / HTTP client

  
scaffold

Repo, CI, tooling

  
ui

Wails shell and frontend

No labels
data-integrity
engine
platform
procurement
remote
scaffold
ui
Milestone
Clear milestone
No items
No milestone
phase-2-integration
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Cordy/cairn-desktop#23
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?