sync: enforce .squirrel-volume markers on remote rclone destinations#170
Merged
Conversation
Bring remote rclone destinations (sftp/s3/b2/gcs) to marker parity with local. The .squirrel-volume marker gate — which refuses a sync when the destination's per-volume root has no marker (likely a wrong or unmounted root) or a marker naming a different volume — previously applied only to local destinations. A misconfigured remote root, where the blast radius of writing into an unknown bucket is largest, went unguarded. - volmark: extract Parse/Marshal as the format's single decoder/encoder; Read/Write now compose them, and the remote path reuses them so a remote marker is byte-identical to a local one. - rclone: add catRemote (read a single object) mapping a definite backend "not found" to errRemoteNotFound; any other read failure refuses without writing, so a reachability blip is never mistaken for a fresh root. - sync: ensureDestinationMarker now dispatches local (filesystem) vs remote (rclone read/copyto through the same overlay the transfer uses). The gate applies across mirror, content-addressed, and packed layouts; the marker sits at the volume root and is already filtered out of every transfer, comparison, and restore. Local content-layout destinations stay ungated (separate parity concern). - docs: README, syncing guide, and SAFETY-AUDIT reconciled — remote markers are enforced; the stale closed-#64 pointer is dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
There was a problem hiding this comment.
Pull request overview
This PR closes the safety gap where remote rclone destinations (sftp, s3, b2, gcs) previously skipped the .squirrel-volume destination marker gate. It extends the existing “refuse unless a correct marker is present (or explicitly bootstrapped with --init)” invariant to remotes, using rclone reads/writes through the same overlay path as transfers (including crypt overlays).
Changes:
- Refactored
volmarkencoding/decoding intoParse/Marshal, withRead/Writecomposing them so local and remote markers are byte-identical. - Added an rclone
catread path and wired remote marker validation/bootstrap into sync flows (mirror + remote content-addressed + packed). - Added/updated tests and documentation to reflect enforced remote markers and remove stale tracking references.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| volmark/volmark.go | Extracts canonical marker marshal/unmarshal helpers to share between local FS and remote rclone paths. |
| sync/sync.go | Adds remote destination marker enforcement (read via rclone cat; write via rclone copyto under --init) and refactors local vs remote marker gating. |
| sync/rclone.go | Introduces catRemote, errRemoteNotFound, and “not found” classification for marker reads. |
| sync/remote_marker_test.go | New tests covering remote marker refusal/bootstrap/mismatch and transient read error behavior. |
| sync/content_addressed.go | Gates remote content-addressed pushes on the destination marker (local content-addressed remains intentionally ungated). |
| sync/content_addressed_test.go | Extends fake rclone shim to support cat, adds fault injection, and seeds remote markers for existing fixtures. |
| sync/packed.go | Gates packed remote pushes on the destination marker. |
| sync/packed_test.go | Adjusts fixture marker seeding to account for crypt overlay suffix behavior. |
| README.md | Updates user docs to state remote rclone destinations now enforce .squirrel-volume markers (parity with local). |
| docs/src/content/docs/guides/syncing.md | Mirrors README changes describing enforced remote markers and the --init bootstrap rule. |
| SAFETY-AUDIT.md | Updates safety audit notes to reflect #64/#150 resolution and remote marker enforcement behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
isNotFoundErr matched a bare "not found" substring, so a DNS "host not
found" or similar network failure was misclassified as an absent marker:
under --init that produced a misleading bootstrap hint and attempted a
marker WRITE against a destination that may simply be unreachable.
Require rclone's canonical absence phrasing ("object not found" /
"directory not found") instead, keeping the case-insensitive match. A
"host not found"/connection error now propagates as a hard error and
refuses the sync, per the refuse-over-wrong-write invariant.
Adds a table-driven isNotFoundErr classification test and a "host not
found" subtest asserting such a read error is not treated as absence and
does not bootstrap the marker.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
Bucket backends (s3/b2/gcs) return an empty body on a zero exit when `rclone cat` addresses a missing object, which cat cannot distinguish from a genuinely empty file. The previous gate inferred absence from cat's error, so on those backends a fresh destination took the err==nil path, volmark.Parse failed on the empty body, and the sync was refused with a confusing parse error — `--init` could never bootstrap a marker on three of the four targeted remote types. Decide absence with a stat (new Rclone.statRemoteExists over `rclone lsjson --stat`) instead: a "null"/empty stat body on a zero exit or a canonical not-found exit is a definite absence (eligible for an --init bootstrap); any other stat failure refuses without writing; a present marker is read via cat and validated, and a mismatch or a corrupt/empty marker is always refused and never overwritten, even under --init. This preserves the refuse-over-wrong-write invariant while making --init functional on bucket backends. The fake-rclone shim gains RCLONE_FAKE_STAT_FAIL and RCLONE_FAKE_BUCKET_ABSENCE (models the bucket zero-exit empty/null reply for a missing object); new tests cover bucket-absence bootstrap, a stat probe error refusing under --init, and a present-marker read error refusing without overwriting. Cannot be verified against a real s3/b2/gcs bucket here (fake shim only) — confirm on a real bucket as part of the #129 shakedown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7
mbertschler
pushed a commit
that referenced
this pull request
Jul 24, 2026
Textual conflicts:
- cmd/squirrel/root.go: keep this branch's config/node/destination
commands alongside main's `squirrel version`.
- cmd/squirrel/agent.go: keep the startup cert-fingerprint attributes (F1)
on top of main's `version` var, which replaced this branch's local
`agentVersion` const.
- sync/{content_addressed,packed}_test.go: keep both sides' tests — this
branch's F20 fresh-start cases and main's dry-run previews (which
replaced the dry-run-refused cases), plus both sets of fake-rclone env
resets.
Semantic conflicts the merge exposed:
- Duplicate rclone-obscure helpers: main's config/rclone_obscure.go
(#153) and this branch's config/obscure.go declared the same symbol.
Consolidated on main's rcloneObscure — it is the merged, golden-pinned
implementation — and repointed resolveCryptSecret at it (it can no
longer fail, so the error path went away). Ported this branch's unique
coverage, the cross-check against a real `rclone reveal`, into main's
test file; the IV-distinctness assertion could not come along, as main
deliberately fixes the IV at zero and documents that trade-off.
- The remote-marker gate (#150/#170) writes .squirrel-volume under the
destination root, which made this branch's "empty root means fresh
start" probe (F20) unreachable: a wiped root is re-bootstrapped with a
marker before any layout guard runs, so it never looks empty again.
remoteRootEmpty now skips volume markers — they are squirrel's own
bookkeeping, not content. Anything else still reads as non-empty and
keeps the guard's refusal. Main's two dry-run layout-flip tests now
seed mirror-era bytes, the same adaptation this branch already applied
to their non-dry-run siblings, so they still describe a genuine layout
conflict rather than a wiped destination.
- The fake rclone shim's `lsf` returned underlying names through a crypt
overlay; real rclone reports decrypted ones. It now strips the data
suffix, so a name-matching caller sees what rclone would.
- F21 is implemented on both sides: main rejects an unsatisfiable
offload_requires at config load, ahead of `config check`'s own
classification. Its test now pins the surviving contract — name the
target, explain it can never gate, exit non-zero.
go vet, go test ./..., and gofmt are clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQUzRmQHqbP2kV9kFrbBHL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
.squirrel-volumemarker guards against pushing to a wrong or unmounted root: on first sync a marker is written under the destination's per-volume directory, and every later sync refuses if it is missing or names a different volume. Until now this gate applied only to local destinations — remote rclone destinations (sftp,s3,b2,gcs) skipped it entirely, exactly where the blast radius (a remote bucket of unknown contents) is largest. This PR closes that gap and drops the dangling closed-#64 follow-up pointer.The guiding invariant: favour refusing over risking a wrong-target write. A marker read that fails for any reason other than a definite "not found" refuses the sync without writing, so a reachability blip can never be mistaken for a fresh root and clobber a marker it could not read.
Changes
volmark: extractedParse/Marshalas the format's single decoder/encoder;Read/Writenow compose them. The remote path reuses them so a remote marker is byte-identical to a local one. Marker format unchanged.sync/rclone.go: addedcatRemote(read one object viarclone cat) that maps a definite backend "not found" (object/dir absent) to a newerrRemoteNotFound; any other read error propagates as a hard failure.sync/sync.go:ensureDestinationMarkernow dispatches — local destinations validate on the filesystem (unchanged), remote destinations read and, with--init, write the marker over rclone through the same overlay the layout's transfers use (so a crypt destination's marker rides the encrypted path). A missing marker is bootstrapped only with--init(refused otherwise); a mismatched marker is always refused.--filter; content/packed enumerate content by hash, never the volume root).README.md,docs/src/content/docs/guides/syncing.md, andSAFETY-AUDIT.md(H7) updated to describe enforced remote markers and drop the stale closed-sync: require .squirrel-volume markers on destination and source to gate against misconfiguration #64 reference.Testing
go build ./...,go vet ./...— clean.go test ./...— all packages pass.golangci-lint run ./...— 0 issues (had to build golangci-lint with the go1.26 toolchain to match go.mod; the packaged binary refused/panicked on go1.26 source).sync/remote_marker_test.go: missing-marker-without---initrefuses (no write, no runs row);--initbootstraps and later syncs validate; mismatched marker always refuses and is never overwritten; a "host not found" / connection-error read refuses even under--initwithout writing; a table-drivenisNotFoundErrclassification test; content-addressed handler wiring for both the refusal and the--initbootstrap.catcase with fault injection.Design decisions to double-check
isNotFoundErr): matches only rclone's canonical absence messages —object not found(fs.ErrorObjectNotFound) anddirectory not found(fs.ErrorDirNotFound), case-insensitively. A "not found"-bearing network error such as DNShost not foundis deliberately not treated as absence: it propagates as a hard error and refuses the sync (never a misleading--inithint, never a marker write against an unreachable host). Pinned by a dedicated classification test. (Tightened from the initial bare-substring match after review.)Closes #150
🤖 Generated with Claude Code
https://claude.ai/code/session_014Skm2Qm6wWUB7dD4CpPTz7