feat: snapshot pipeline from zsh hook to APFS clonefile store - #1
feat: snapshot pipeline from zsh hook to APFS clonefile store#1mickamy wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the core pipeline for mulligan, a utility that automatically snapshots files before destructive shell commands run. It introduces packages for parsing commands (detect), determining snapshot scopes (scope), managing on-disk generations (store), orchestrating the snapshotting process (snap), and utilizing macOS clonefile(2) for zero-copy APFS cloning (clonefile). Feedback on the changes highlights two critical compilation issues in the clonefile package: a type mismatch on Darwin when converting srcFS.Fstypename to a byte slice, and a build failure on non-Darwin platforms due to the lack of fallback stub implementations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if unix.ByteSliceToString(srcFS.Fstypename[:]) != "apfs" { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
- Severity: High
- Issue:
srcFS.Fstypenameis of type[16]int8on Darwin, sosrcFS.Fstypename[:]produces a slice of type[]int8. Passing this tounix.ByteSliceToString, which expects[]byte, will cause a compilation error. - Recommendation: Convert the
int8array to abyteslice before callingunix.ByteSliceToString.
var fstypeBuf [16]byte
for i, b := range srcFS.Fstypename {
fstypeBuf[i] = byte(b)
}
if unix.ByteSliceToString(fstypeBuf[:]) != "apfs" {
return false, nil
}References
- Issues must be output strictly in the format: - Severity: ... - Issue: ... - Recommendation: ... (link)
There was a problem hiding this comment.
This is a false positive. Fstypename is [16]int8 in the standard library's syscall.Statfs_t, but this code uses golang.org/x/sys/unix.Statfs_t, where it is [16]byte on Darwin:
$ go doc golang.org/x/sys/unix.Statfs_t | grep Fstypename
Fstypename [16]byteThe package compiles and its tests pass on macOS (see the CI run on macos-latest).
| @@ -0,0 +1,42 @@ | |||
| //go:build darwin | |||
There was a problem hiding this comment.
- Severity: Medium
- Issue: The
clonefilepackage only contains files with the//go:build darwinconstraint. When compiling the project on non-Darwin platforms (such as Linux or Windows), this will cause a "build constraints exclude all Go files" compilation error, which prevents the CLI from building even if the platform is not supported. - Recommendation: Add a fallback file (e.g.,
clonefile_fallback.go) with//go:build !darwinthat provides stub implementations ofCloneandSupportedreturning appropriate errors orfalse.
References
- Issues must be output strictly in the format: - Severity: ... - Issue: ... - Recommendation: ... (link)
There was a problem hiding this comment.
This is intentional: mulligan is macOS-only for now because the snapshot mechanism is APFS clonefile(2) itself, so a non-Darwin build has nothing meaningful to do. Release builds (.goreleaser.yaml) and CI both target Darwin only.
A Linux port would be backed by reflink (copy_file_range on btrfs/XFS), at which point this package gets a real _linux.go implementation rather than an error-returning stub.
There was a problem hiding this comment.
Pull request overview
This PR implements the “capture” half of mulligan: detecting destructive shell commands, deciding what to snapshot, and cloning targets into a per-project on-disk generation store via APFS clonefile(2) (with zsh preexec hook + CLI wiring).
Changes:
- Added destructive-command detection (
internal/detect) and a 3-tier scope decision engine (internal/scope) to select snapshot targets. - Implemented a crash-tolerant generation store with atomic metadata commit (
internal/store) and orchestration to clone selected paths into the store (internal/snap). - Added a darwin-only
clonefile(2)wrapper (internal/clonefile), CLI commands (snap,hook zsh), and moved CI tomacos-latest.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/store/store.go | New on-disk store layout: per-project directory + numbered generations + atomic meta.json commit. |
| internal/store/store_test.go | Unit tests for default root selection, project naming, generation allocation/visibility, and commit atomicity. |
| internal/snap/snap.go | New snapshot orchestration pipeline: detect → scope decide → APFS support filter → clone → commit metadata. |
| internal/snap/snap_test.go | Integration-style tests validating snapshot survival and scope behaviors. |
| internal/scope/scope.go | Scope decision logic (paths → cwd-if-small → git-unprotected-if-large) with path resolution utilities. |
| internal/scope/scope_test.go | Tests for the 3-tier scope strategy, including git status parsing behavior. |
| internal/hook/hook.go | Zsh preexec hook snippet generator for invoking mulligan snap --cmd "$3". |
| internal/detect/lex.go | Command-line segmentation/tokenization for POSIX-ish quoting and compound commands. |
| internal/detect/detect.go | Destructive command detection and path extraction (rm/mv/git/sed) with “opaque target” fallback. |
| internal/detect/detect_test.go | Table tests for detection behavior across commands, wrappers, redirects, and compounds. |
| internal/clonefile/clonefile.go | Darwin-only clonefile(2) wrapper + same-APFS-volume support check. |
| internal/clonefile/clonefile_test.go | Tests for file/dir/symlink cloning and APFS support detection. |
| internal/cli/cli.go | Implements snap and hook zsh subcommands with a timeout for preexec usage. |
| internal/cli/cli_test.go | Extends CLI tests, including an end-to-end destructive snap flow validating store output. |
| go.mod | Adds dependency on golang.org/x/sys for darwin clonefile syscalls. |
| go.sum | Adds checksums for golang.org/x/sys. |
| .golangci.yaml | Adjusts lint configuration (wrapcheck ignore globs; disables gosec in tests). |
| .github/workflows/ci.yaml | Runs lint/test/build on macos-latest to support darwin-only clonefile behavior. |
| // Snapshots mirror the absolute path under the data dir, so no | ||
| // path needs to be relative to the project root. | ||
| dst := filepath.Join(gen.DataDir(), p) | ||
|
|
||
| if err := os.MkdirAll(filepath.Dir(dst), 0o700); err != nil { |
There was a problem hiding this comment.
This is a false positive: Go's filepath.Join does not discard earlier elements when a later element is absolute (that is Python's os.path.join behavior). It concatenates all elements and runs Clean:
filepath.Join("/store/data", "/Users/x/build")
// => "/store/data/Users/x/build"The absolute-path-mirror layout under gen.DataDir() is intentional and is verified by TestSnap_NamedPath and the CLI integration test TestRun_SnapDestructive, which assert the snapshot content exists under the store after the original is removed. CI on macos-latest is green.
| import ( | ||
| "context" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| "github.com/mickamy/mulligan/internal/clonefile" | ||
| "github.com/mickamy/mulligan/internal/detect" | ||
| "github.com/mickamy/mulligan/internal/scope" | ||
| "github.com/mickamy/mulligan/internal/store" | ||
| ) |
There was a problem hiding this comment.
No change needed: this recommendation depends on the filepath.Join concern above, which is a false positive (Go's Join keeps earlier elements even when a later element is absolute), so no strings-based rewrite is required.
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/mickamy/mulligan/internal/snap" | ||
| "github.com/mickamy/mulligan/internal/store" | ||
| ) |
There was a problem hiding this comment.
No change needed: this recommendation depends on the filepath.Join concern above, which is a false positive (Go's Join keeps earlier elements even when a later element is absolute), so no strings-based rewrite is required.
| // The snapshot mirrors the absolute path under the data dir. | ||
| snapped := filepath.Join(gen.DataDir(), cwd, "build", "sub", "b.txt") | ||
| if got := readFile(t, snapped); got != "b" { |
There was a problem hiding this comment.
No change needed: this recommendation depends on the filepath.Join concern above, which is a false positive (Go's Join keeps earlier elements even when a later element is absolute), so no strings-based rewrite is required.
| } | ||
|
|
||
| snapped := filepath.Join(gen.DataDir(), cwd, "config.yaml") | ||
| if got := readFile(t, snapped); got != "v: 1" { | ||
| t.Errorf("snapshot content = %q, want %q", got, "v: 1") | ||
| } |
There was a problem hiding this comment.
No change needed: this recommendation depends on the filepath.Join concern above, which is a false positive (Go's Join keeps earlier elements even when a later element is absolute), so no strings-based rewrite is required.
| snapped := filepath.Join(gen.DataDir(), cwd, "a.txt") | ||
| if _, err := os.Stat(snapped); err != nil { | ||
| t.Errorf("snapshot missing: %v", err) | ||
| } |
There was a problem hiding this comment.
No change needed: this recommendation depends on the filepath.Join concern above, which is a false positive (Go's Join keeps earlier elements even when a later element is absolute), so no strings-based rewrite is required.
| sum := sha256.Sum256([]byte(abs)) | ||
| name := filepath.Base(abs) + "-" + hex.EncodeToString(sum[:4]) | ||
|
|
||
| return Project{dir: filepath.Join(s.root, name), root: abs}, nil | ||
| } |
There was a problem hiding this comment.
Partially right. The "outside the store root" part does not happen — Go's filepath.Join keeps earlier elements even when a later one is absolute, so Join(s.root, "/-abc123") still lands under s.root. But filepath.Base("/") returning / is real and would produce an awkward -<hash> directory name, so the base name is now sanitized to root for the filesystem root: d5a7557.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the core snapshotting pipeline for mulligan, a tool designed to undo destructive shell commands. It introduces command detection, scope decision strategies, an APFS-based zero-copy file cloning mechanism, an on-disk generation store, and a Zsh preexec hook. A high-severity issue was identified in the Supported function of the clonefile package, where a compilation error occurs due to an invalid slice conversion of Fstypename on macOS, and a logical bug exists where symlinks are followed instead of being evaluated directly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func Supported(src, dstDir string) (bool, error) { | ||
| var srcFS, dstFS unix.Statfs_t | ||
|
|
||
| if err := unix.Statfs(src, &srcFS); err != nil { | ||
| return false, fmt.Errorf("statfs %s: %w", src, err) | ||
| } | ||
| if err := unix.Statfs(dstDir, &dstFS); err != nil { | ||
| return false, fmt.Errorf("statfs %s: %w", dstDir, err) | ||
| } | ||
|
|
||
| if unix.ByteSliceToString(srcFS.Fstypename[:]) != "apfs" { | ||
| return false, nil | ||
| } | ||
|
|
||
| return srcFS.Fsid == dstFS.Fsid, nil | ||
| } |
There was a problem hiding this comment.
- Severity: High
- Issue: The current implementation of
Supportedhas two major issues:
- It will fail to compile because
srcFS.Fstypenameis of type[16]int8on macOS, which cannot be sliced into[]byteforunix.ByteSliceToString. - It uses
unix.Statfsonsrc, which follows symlinks. Sinceclonefileis called withunix.CLONE_NOFOLLOW, symlinks themselves are cloned rather than their targets. If a symlink points to a target on a different volume,Supportedwill incorrectly check the target's volume instead of the symlink's volume.
- Recommendation: Use
unix.Lstatonsrcandunix.StatondstDirto compare their device IDs (Dev), and then verify thatdstDiris on an APFS volume usingunix.Statfs. Convert theFstypenameint8array to a string manually to ensure compilation.
func Supported(src, dstDir string) (bool, error) {
var srcStat, dstStat unix.Stat_t
if err := unix.Lstat(src, &srcStat); err != nil {
return false, fmt.Errorf("lstat %s: %w", src, err)
}
if err := unix.Stat(dstDir, &dstStat); err != nil {
return false, fmt.Errorf("stat %s: %w", dstDir, err)
}
if srcStat.Dev != dstStat.Dev {
return false, nil
}
var dstFS unix.Statfs_t
if err := unix.Statfs(dstDir, &dstFS); err != nil {
return false, fmt.Errorf("statfs %s: %w", dstDir, err)
}
var fstype []byte
for _, b := range dstFS.Fstypename {
if b == 0 {
break
}
fstype = append(fstype, byte(b))
}
return string(fstype) == "apfs", nil
}References
- Output Format: If there are any issues, you must output them strictly in the following format. Do not include any greetings or introductory explanations. - Severity: [High / Medium / Low] - Issue: (Briefly explain why it is a problem) - Recommendation: (Suggest how to fix or improve it) (link)
| status, rel := entry[:2], entry[3:] | ||
|
|
||
| if status[0] == 'R' || status[0] == 'C' { | ||
| i++ // the next field is the rename/copy source | ||
| } | ||
|
|
||
| p := filepath.Join(root, rel) |
| //nolint:dupword // add-zsh-hook legitimately appears on consecutive lines | ||
| return `# mulligan: snapshot files before destructive commands | ||
| _mulligan_preexec() { | ||
| command mulligan snap --cmd "$3" 2>/dev/null | ||
| } | ||
| autoload -Uz add-zsh-hook | ||
| add-zsh-hook preexec _mulligan_preexec | ||
| ` |
| //go:build darwin | ||
|
|
||
| package clonefile | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "golang.org/x/sys/unix" |
Summary
Implements the "capture" half of mulligan: a zsh preexec hook that snapshots files via APFS clonefile(2) before destructive commands run.
internal/clonefile: clonefile(2) wrapper with APFS same-volume detectioninternal/store: generation store with two-phase commit (crash-safe; orphans are left forprune)internal/detect: destructive command detection (rm,git clean/reset --hard/checkout --/restore,sed -i,mv) with path extraction; falls back to opaque on shell expansion ($VAR, globs)internal/scope: three-tier scope decision (named paths > whole cwd if small > git-unprotected files in large repos)internal/snap: orchestration; per-path failures never block the user's commandmulligan snap --cmdandmulligan hook zsh; install witheval "$(mulligan hook zsh)"CI now runs on macos-latest since clonefile(2) is darwin-only.
Test plan
make test(all packages, including a CLI-level integration test against a real APFS volume)snapbeforerm -rf build, snapshot survives with correct content and metadataThe restore half (
restore,diff, the no-arg undo flow) comes in a follow-up PR.