Task 11: Rename detection via FileID #11

New issue

Closed

opened 2026-09-10 17:16:33 +00:00 by Cordy · 1 comment

Cordy commented

2026-09-10 17:16:33 +00:00

Owner

Depends on Task 10.

Goal

Collapse a (OpDeleteRemote, OpUpload) pair into a single OpMoveRemote when both refer to the same underlying file.

Why

Without this, renaming a 2 GB folder deletes 2 GB from the server and re-uploads 2 GB. With it, the same rename is one MOVE request.

Files

Create: internal/sync/rename.go, internal/sync/rename_test.go

Produces

func DetectRenames(ops []Operation, currentIDs, priorIDs map[string]string) []Operation

currentIDs — currently-present local path → vfs.FileInfo.FileID
priorIDs — last-synced path → state.Entry.LocalFileID recorded at that time

Identity comes from the filesystem's FileID (inode on Unix, file index on Windows), which survives a rename. An empty FileID means "unknown" and must never match — otherwise two unrelated unknown files would be mistaken for a move.

Steps

Write the failing test:

package sync

import "testing"

func TestDetectLocalRenameByFileID(t *testing.T) {
	ops := []Operation{
		{Op: OpDeleteRemote, Path: "old.txt"},
		{Op: OpUpload, Path: "new.txt"},
	}
	got := DetectRenames(ops,
		map[string]string{"new.txt": "42"},
		map[string]string{"old.txt": "42"})

	if len(got) != 1 {
		t.Fatalf("expected 1 operation after collapsing, got %d: %+v", len(got), got)
	}
	if got[0].Op != OpMoveRemote || got[0].From != "old.txt" || got[0].Path != "new.txt" {
		t.Errorf("got %+v, want move-remote old.txt -> new.txt", got[0])
	}
}

func TestNoRenameWhenFileIDDiffers(t *testing.T) {
	ops := []Operation{
		{Op: OpDeleteRemote, Path: "old.txt"},
		{Op: OpUpload, Path: "new.txt"},
	}
	got := DetectRenames(ops,
		map[string]string{"new.txt": "99"},
		map[string]string{"old.txt": "42"})
	if len(got) != 2 {
		t.Errorf("different FileIDs must not collapse into a move, got %+v", got)
	}
}

func TestNoRenameWhenFileIDEmpty(t *testing.T) {
	ops := []Operation{
		{Op: OpDeleteRemote, Path: "old.txt"},
		{Op: OpUpload, Path: "new.txt"},
	}
	got := DetectRenames(ops,
		map[string]string{"new.txt": ""},
		map[string]string{"old.txt": ""})
	if len(got) != 2 {
		t.Errorf("empty FileIDs must not match, got %+v", got)
	}
}

func TestUnrelatedOpsPassThrough(t *testing.T) {
	ops := []Operation{{Op: OpDownload, Path: "x.txt"}}
	if got := DetectRenames(ops, nil, nil); len(got) != 1 || got[0].Op != OpDownload {
		t.Errorf("unrelated ops must pass through unchanged, got %+v", got)
	}
}

Run them, confirm they fail.
Implement. Index uploads by the FileID of the file now at that path, then for each OpDeleteRemote look up the prior FileID and check for a matching upload. Emit the move and drop both originals. Handle nil maps.
Run tests, confirm they pass.
Commit: git commit -s -m "feat(sync): collapse delete+upload into move via FileID"

Acceptance criteria

A matching FileID collapses two operations into one OpMoveRemote with From and Path set.
Differing or empty FileIDs never collapse.
Operations that are not part of a detected pair pass through untouched and in order.

Depends on Task 10. ## Goal Collapse a `(OpDeleteRemote, OpUpload)` pair into a single `OpMoveRemote` when both refer to the same underlying file. ## Why Without this, renaming a 2 GB folder deletes 2 GB from the server and re-uploads 2 GB. With it, the same rename is one MOVE request. ## Files - Create: `internal/sync/rename.go`, `internal/sync/rename_test.go` ## Produces ```go func DetectRenames(ops []Operation, currentIDs, priorIDs map[string]string) []Operation ``` - `currentIDs` — currently-present local path → `vfs.FileInfo.FileID` - `priorIDs` — last-synced path → `state.Entry.LocalFileID` recorded at that time Identity comes from the filesystem's `FileID` (inode on Unix, file index on Windows), which survives a rename. **An empty `FileID` means "unknown" and must never match** — otherwise two unrelated unknown files would be mistaken for a move. ## Steps - [ ] **Write the failing test:** ```go package sync import "testing" func TestDetectLocalRenameByFileID(t *testing.T) { ops := []Operation{ {Op: OpDeleteRemote, Path: "old.txt"}, {Op: OpUpload, Path: "new.txt"}, } got := DetectRenames(ops, map[string]string{"new.txt": "42"}, map[string]string{"old.txt": "42"}) if len(got) != 1 { t.Fatalf("expected 1 operation after collapsing, got %d: %+v", len(got), got) } if got[0].Op != OpMoveRemote || got[0].From != "old.txt" || got[0].Path != "new.txt" { t.Errorf("got %+v, want move-remote old.txt -> new.txt", got[0]) } } func TestNoRenameWhenFileIDDiffers(t *testing.T) { ops := []Operation{ {Op: OpDeleteRemote, Path: "old.txt"}, {Op: OpUpload, Path: "new.txt"}, } got := DetectRenames(ops, map[string]string{"new.txt": "99"}, map[string]string{"old.txt": "42"}) if len(got) != 2 { t.Errorf("different FileIDs must not collapse into a move, got %+v", got) } } func TestNoRenameWhenFileIDEmpty(t *testing.T) { ops := []Operation{ {Op: OpDeleteRemote, Path: "old.txt"}, {Op: OpUpload, Path: "new.txt"}, } got := DetectRenames(ops, map[string]string{"new.txt": ""}, map[string]string{"old.txt": ""}) if len(got) != 2 { t.Errorf("empty FileIDs must not match, got %+v", got) } } func TestUnrelatedOpsPassThrough(t *testing.T) { ops := []Operation{{Op: OpDownload, Path: "x.txt"}} if got := DetectRenames(ops, nil, nil); len(got) != 1 || got[0].Op != OpDownload { t.Errorf("unrelated ops must pass through unchanged, got %+v", got) } } ``` - [ ] **Run them, confirm they fail.** - [ ] **Implement.** Index uploads by the FileID of the file now at that path, then for each `OpDeleteRemote` look up the prior FileID and check for a matching upload. Emit the move and drop both originals. Handle `nil` maps. - [ ] **Run tests, confirm they pass.** - [ ] **Commit:** `git commit -s -m "feat(sync): collapse delete+upload into move via FileID"` ## Acceptance criteria - A matching FileID collapses two operations into one `OpMoveRemote` with `From` and `Path` set. - Differing or empty FileIDs never collapse. - Operations that are not part of a detected pair pass through untouched and in order.

Cordy added this to the phase-1-engine milestone

2026-09-10 17:16:33 +00:00

Cordy added the

engine

label

2026-09-10 17:16:33 +00:00

Cordy referenced this issue

2026-09-10 21:00:18 +00:00

Task 5: Filesystem interface and in-memory implementation #5

Cordy commented

2026-09-10 23:04:08 +00:00

Author

Owner

Done

What was built

DetectRenames(ops []Operation, currentIDs, priorIDs map[string]string) []Operation in internal/sync/rename.go, collapsing a matched (OpDeleteRemote, OpUpload) pair into one OpMoveRemote.
Two-pass pairing: pass 1 decides every delete→upload pairing over the whole slice (each upload consumed at most once); pass 2 emits moves/pass-throughs — pairing is independent of input order.
Empty FileID (nil map, missing key, or "") never matches on either side.
Fix round 1 addressed two review findings: upload-before-delete no longer duplicates the upload (F1), and two deletes sharing one FileID no longer both consume the same upload (F2).

Tests

8 unit tests in internal/sync/rename_test.go (4 verbatim issue tests + 2 Task-5/CV4 nil-map tests + 2 fix-round regression tests for F1/F2), all passing.
go vet ./... and go test -count=1 ./... clean across all 4 packages; gofmt -l . empty; go mod tidy no diff.
CI run #11 on linux/arm64 — green.

Acceptance criteria

"A matching FileID collapses two operations into one OpMoveRemote with From and Path set" — met; TestDetectLocalRenameByFileID plus F1's TestDetectRenameWhenUploadPrecedesDelete (order no longer matters).
"Differing or empty FileIDs never collapse" — met; TestNoRenameWhenFileIDDiffers, TestNoRenameWhenFileIDEmpty, plus nil/missing-key variants.
"Operations that are not part of a detected pair pass through untouched and in order" — met; TestUnrelatedOpsPassThrough plus F2's TestSharedFileIDUploadConsumedByAtMostOneDelete (leftover delete keeps its position/value).

Rulings

R12 (brief): commit subjects use the exact issue text, always git commit -s, and carry Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> — DCO is non-negotiable.
progress.md Ruling (F4): identity by FileID alone can invent a move on inode reuse or a rename+edit; DetectRenames stands as-is (no hash in this signature) — carried into #12: after OpMoveRemote, compare local hash to the From row's ContentHash and re-upload on mismatch, with defined fallbacks if Move/Put fail.
progress.md (CV1, parked/satisfied): red step reproduced in a scratch clone of 7ef654a — compile fails with undefined: DetectRenames at the exact report lines, green after restoring the file.
progress.md (CV2, parked): op order needs no #12 ruling — AC1 has no ordering precondition and the F1 fix makes pairing order-independent for any feed order.
progress.md (CV3, parked): hazard rate unverifiable but no ruling needed — F2 caps upload consumption at one delete regardless of rate; F4 carry-forward covers inode reuse at any rate.

Deferred

F3 (minor): no test yet mixes a collapsed pair with unrelated pass-through ops in one slice.
F4 (plan-mandated content-hash-after-move check): explicitly deferred to #12 per the ruling above.
F5 (minor, prose only): report's superseded "order-independent" claim left as history, no code impact.

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

**Done** - [feat(sync): collapse delete+upload into move via FileID](http://192.168.10.245/Cordy/cairn-desktop/commit/7ef654a79a0bc08f6e1ffbff0dae62ffc9a9a51d) - [fix(sync): pair delete+upload renames independent of ops order](http://192.168.10.245/Cordy/cairn-desktop/commit/024ea527dd252438c7cd1b104987d7da4db3852c) **What was built** - `DetectRenames(ops []Operation, currentIDs, priorIDs map[string]string) []Operation` in `internal/sync/rename.go`, collapsing a matched `(OpDeleteRemote, OpUpload)` pair into one `OpMoveRemote`. - Two-pass pairing: pass 1 decides every delete→upload pairing over the whole slice (each upload consumed at most once); pass 2 emits moves/pass-throughs — pairing is independent of input order. - Empty FileID (nil map, missing key, or `""`) never matches on either side. - Fix round 1 addressed two review findings: upload-before-delete no longer duplicates the upload (F1), and two deletes sharing one FileID no longer both consume the same upload (F2). **Tests** - 8 unit tests in `internal/sync/rename_test.go` (4 verbatim issue tests + 2 Task-5/CV4 nil-map tests + 2 fix-round regression tests for F1/F2), all passing. - `go vet ./...` and `go test -count=1 ./...` clean across all 4 packages; `gofmt -l .` empty; `go mod tidy` no diff. - CI run [#11](http://192.168.10.245/Cordy/cairn-desktop/actions/runs/11) on linux/arm64 — green. **Acceptance criteria** - "A matching FileID collapses two operations into one `OpMoveRemote` with `From` and `Path` set" — met; `TestDetectLocalRenameByFileID` plus F1's `TestDetectRenameWhenUploadPrecedesDelete` (order no longer matters). - "Differing or empty FileIDs never collapse" — met; `TestNoRenameWhenFileIDDiffers`, `TestNoRenameWhenFileIDEmpty`, plus nil/missing-key variants. - "Operations that are not part of a detected pair pass through untouched and in order" — met; `TestUnrelatedOpsPassThrough` plus F2's `TestSharedFileIDUploadConsumedByAtMostOneDelete` (leftover delete keeps its position/value). **Rulings** - R12 (brief): commit subjects use the exact issue text, always `git commit -s`, and carry `Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>` — DCO is non-negotiable. - progress.md Ruling (F4): identity by FileID alone can invent a move on inode reuse or a rename+edit; `DetectRenames` stands as-is (no hash in this signature) — carried into #12: after `OpMoveRemote`, compare local hash to the `From` row's `ContentHash` and re-upload on mismatch, with defined fallbacks if `Move`/`Put` fail. - progress.md (CV1, parked/satisfied): red step reproduced in a scratch clone of `7ef654a` — compile fails with `undefined: DetectRenames` at the exact report lines, green after restoring the file. - progress.md (CV2, parked): op order needs no #12 ruling — AC1 has no ordering precondition and the F1 fix makes pairing order-independent for any feed order. - progress.md (CV3, parked): hazard rate unverifiable but no ruling needed — F2 caps upload consumption at one delete regardless of rate; F4 carry-forward covers inode reuse at any rate. **Deferred** - F3 (minor): no test yet mixes a collapsed pair with unrelated pass-through ops in one slice. - F4 (plan-mandated content-hash-after-move check): explicitly deferred to #12 per the ruling above. - F5 (minor, prose only): report's superseded "order-independent" claim left as history, no code impact. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._

Cordy closed this issue

2026-09-10 23:04:13 +00:00