From 3696436658236d3af7bafc3962e647d1f17c6f98 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:00:39 +0000 Subject: [PATCH 1/8] fix: gate the pre-commit secret scan on regular files and the stageable set The changed-file scan opened paths with a symlink-following read, so an untracked repo symlink pointing at ~/.ssh/id_ed25519 was read in full, breaching the ssh boundary, and a dirty submodule or symlink-to-directory EISDIR-wedged every sync fail-closed. Changed paths are now Lstat-gated (git commits a symlink as its link text, which the push-range blob scan covers; a gitlink's content never enters the push range). The collapsed untracked-directory walk is replaced by git ls-files --others --exclude-standard, so gitignored files and nested .git trees no longer false-block a sync that would never publish them. Assisted-by: Claude:claude-fable-5 --- .gitleaks.toml | 3 + cmd/sync.go | 111 +++++++++++-------- cmd/sync_round9_test.go | 234 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 302 insertions(+), 46 deletions(-) create mode 100644 cmd/sync_round9_test.go diff --git a/.gitleaks.toml b/.gitleaks.toml index 7601de4..5c462ce 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -23,6 +23,9 @@ paths = [ # A fake PEM fixture proving the pre-commit secret gate still scans a file's # content after consuming a rename entry's origin path field. '''cmd/sync_rename_scan_test\.go$''', + # Fake key/token fixtures proving the pre-commit secret gate never follows + # symlinks, skips gitlinks, and scans only what `git add -A` would stage. + '''cmd/sync_round9_test\.go$''', # v0.7.0 config-plugin secret tests: credential-shaped fixtures that exercise # the git/tmux/npm token recognisers and prove a literal token never reaches # the shared repo. Fake tokens only; every non-test file stays fully scanned. diff --git a/cmd/sync.go b/cmd/sync.go index 0dd2c92..6184f6b 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -1121,69 +1121,88 @@ func scanWorktreeForSecret(repo string) (string, bool, error) { // the default -unormal (the same shape backupOutOfBand walks for backup). // Reading it as a file EISDIRs — which is neither a deletion nor a // not-exist — so without this branch the fail-closed abort below wedges - // every sync after a capture creates a new repo subdirectory. Walk it and - // gate every regular file inside; symlinks are committed as link targets, - // not content, and stay covered by the push-range scan. FAIL CLOSED on a - // walk error: a file we cannot enumerate is a file we cannot scan. + // every sync after a capture creates a new repo subdirectory. Enumerate it + // and gate every regular file inside. FAIL CLOSED on an enumeration error: + // a file we cannot list is a file we cannot scan. if strings.HasSuffix(path, "/") { - var hit string - werr := filepath.Walk(filepath.Join(repo, path), func(p string, info os.FileInfo, werr error) error { - if werr != nil { - // A path that vanished mid-walk is the same rename/delete - // race the single-file branch below tolerates; anything - // else stays fail-closed. - if os.IsNotExist(werr) { - return nil - } - return werr - } - if hit != "" || !info.Mode().IsRegular() { - return nil - } - rel, rerr := filepath.Rel(repo, p) - if rerr != nil { - return rerr + // Enumerate the set `git add -A` would actually STAGE, not everything on + // disk: `ls-files --others --exclude-standard` applies the ignore rules, + // so a gitignored file (ferry's own init writes an unanchored `local/` + // pattern) is never scanned, and a NESTED repository arrives as a single + // directory entry — git records it as a gitlink and never commits its + // contents, so its `.git/**` must not be scanned either. The pathspec is + // `:(literal)` so a directory whose name holds glob characters enumerates + // itself and not a wildcard match. FAIL CLOSED if the listing errors. + listing, lerr := gitSync(repo, "ls-files", "--others", "--exclude-standard", "-z", "--", ":(literal)"+path) + if lerr != nil { + return "", false, fmt.Errorf("could not scan the untracked directory %q: %s", ghcli.Redact(path), ghcli.Redact(strings.TrimSpace(listing))) + } + for _, rel := range strings.Split(listing, "\x00") { + // The empty trailing field after the final NUL, and a nested + // repository's single directory-shaped entry, carry no content. + if rel == "" || strings.HasSuffix(rel, "/") { + continue } rel = filepath.ToSlash(rel) if secretInPath(rel) { - hit = rel - return nil + return rel, true, nil } - data, rerr := os.ReadFile(p) - if rerr != nil { - if os.IsNotExist(rerr) { - return nil - } - return rerr + blocked, serr := scanRepoFileForSecret(repo, rel) + if serr != nil { + return "", false, fmt.Errorf("could not scan the untracked directory %q: %s", ghcli.Redact(path), ghcli.Redact(serr.Error())) } - if secret.IsBlockedFromRepo(string(data)) { - hit = rel + if blocked { + return rel, true, nil } - return nil - }) - if werr != nil { - return "", false, fmt.Errorf("could not scan the untracked directory %q: %s", ghcli.Redact(path), ghcli.Redact(werr.Error())) - } - if hit != "" { - return hit, true, nil } continue } - data, err := os.ReadFile(filepath.Join(repo, path)) - if err != nil { - if os.IsNotExist(err) { - // A rename/delete race: the path is gone, nothing to scan. - continue - } - return "", false, fmt.Errorf("could not read changed file %q: %w", ghcli.Redact(path), err) + blocked, serr := scanRepoFileForSecret(repo, path) + if serr != nil { + return "", false, fmt.Errorf("could not read changed file %q: %w", ghcli.Redact(path), serr) } - if secret.IsBlockedFromRepo(string(data)) { + if blocked { return path, true, nil } } return "", false, nil } +// scanRepoFileForSecret gates ONE repo-relative path's CONTENT, scanning only a +// REGULAR file. The mode is taken with Lstat, never by opening the path: +// - a SYMLINK is committed as its link TEXT, never the target's bytes, so +// following it would read a file the repo never carries — a link at +// `notes -> ~/.ssh/id_ed25519` would make sync read the private key, which the +// ~/.ssh boundary forbids outright. The link text itself stays covered by the +// push-range blob scan. +// - a DIRECTORY-shaped entry has no content to scan: a gitlink/submodule is +// recorded as type `commit`, which the push-range scan already skips, and its +// files are never committed by this repo. +// +// A path that vanished since `git status` is a rename/delete race, not a scan +// failure. Any OTHER stat or read error is returned so the caller FAILS CLOSED. +func scanRepoFileForSecret(repo, rel string) (bool, error) { + p := filepath.Join(repo, rel) + info, serr := os.Lstat(p) + if serr != nil { + if os.IsNotExist(serr) { + return false, nil + } + return false, serr + } + if !info.Mode().IsRegular() { + return false, nil + } + data, rerr := os.ReadFile(p) + if rerr != nil { + if os.IsNotExist(rerr) { + return false, nil + } + return false, rerr + } + return secret.IsBlockedFromRepo(string(data)), nil +} + // rollback restores the snapshot and returns the appropriate error. On a CLEAN // restore it returns cause (the original failure — machine unchanged). If restore // FAILS, it returns a data-safety error that names WHERE the user's tracked work is diff --git a/cmd/sync_round9_test.go b/cmd/sync_round9_test.go new file mode 100644 index 0000000..47fd647 --- /dev/null +++ b/cmd/sync_round9_test.go @@ -0,0 +1,234 @@ +package cmd + +// Round-9 regressions in the pre-commit worktree secret gate (scanWorktreeForSecret), +// all driving the real scan against real git repos: +// - CRITICAL: the single-file read followed SYMLINKS, so an untracked repo symlink +// pointing at ~/.ssh/id_ed25519 made sync READ the private key — a breach of the +// "~/.ssh is untouchable" boundary. Git commits a symlink as its link TEXT, never +// the target's bytes, so reading the target is both wrong and unnecessary. +// - MAJOR: a directory-shaped entry emitted WITHOUT a trailing slash (a dirty +// submodule ` M sub`, an untracked symlink-to-directory `?? link`) EISDIRed the +// same read. EISDIR is neither a not-exist nor a deletion, so the gate failed +// closed and wedged every sync with advice a directory can never satisfy. +// - MAJOR: the `?? dir/` branch walked the WHOLE directory, scanning files +// `git add -A` would never stage — gitignored paths (ferry's own init writes an +// unanchored `local/` pattern) and a nested repository's `.git/**` — so an +// ignored token or a nested repo's credential-bearing remote URL false-blocked +// the sync. + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// r9Repo is r3Repo plus the git-config neutralisation the collapsed-directory +// entries depend on: a developer's status.showUntrackedFiles=all would list +// per-file entries and never produce the `?? dir/` shape these tests pin. +func r9Repo(t *testing.T) string { + t.Helper() + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + return r3Repo(t) +} + +// CRITICAL: an untracked symlink whose TARGET lives outside the repo (the +// ~/.ssh/id_ed25519 shape) must not be dereferenced by the gate. Git stages the +// link text, so the target's bytes never enter a commit and the push-range blob +// scan already covers what does; following it would read a file the boundary +// declares untouchable — and, as here, block the sync on content that is never +// going anywhere. +func TestScanWorktreeDoesNotFollowUntrackedSymlink(t *testing.T) { + repo := r9Repo(t) + // A private-key-shaped file OUTSIDE the repo, standing in for ~/.ssh/id_ed25519. + outside := filepath.Join(t.TempDir(), "id_ed25519") + if err := os.WriteFile(outside, []byte(fakeRound9Key), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(repo, "notes")); err != nil { + t.Fatal(err) + } + + path, found, err := scanWorktreeForSecret(repo) + if err != nil { + t.Fatalf("scanWorktreeForSecret errored on an untracked symlink: %v", err) + } + if found { + t.Fatalf("the gate FOLLOWED a symlink out of the repo and blocked on its target (path %q) — sync read a file it must never open", path) + } +} + +// A symlink to a DIRECTORY is a plain `?? link` entry — directory-shaped with no +// trailing slash — so reading it as a file returns EISDIR, which is neither a +// not-exist nor a deletion. Before the lstat gate that wedged every sync. +func TestScanWorktreeSkipsUntrackedSymlinkToDirectory(t *testing.T) { + repo := r9Repo(t) + target := filepath.Join(repo, "realdir") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "a.txt"), []byte("harmless\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(repo, "dlink")); err != nil { + t.Fatal(err) + } + + path, found, err := scanWorktreeForSecret(repo) + if err != nil { + t.Fatalf("an untracked symlink-to-directory wedged the gate: %v", err) + } + if found { + t.Fatalf("no secret was seeded, but the scan blocked on %q", path) + } +} + +// A dirty gitlink (embedded repository whose HEAD moved) is reported as ` M sub` +// — again directory-shaped without a trailing slash. A gitlink's CONTENT is never +// committed (ls-tree records it as type `commit`, which the push-range scan +// already skips), so skipping it loses no coverage. +func TestScanWorktreeSkipsDirtyGitlinkEntry(t *testing.T) { + repo := r9Repo(t) + sub := filepath.Join(repo, "sub") + testGit(t, repo, "init", "-q", "-b", "main", "sub") + if err := os.WriteFile(filepath.Join(sub, "x.txt"), []byte("one\n"), 0o644); err != nil { + t.Fatal(err) + } + testGit(t, sub, "add", "-A") + testGit(t, sub, "commit", "-qm", "sub one") + testGit(t, repo, "add", "sub") // records a gitlink, not the contents + testGit(t, repo, "commit", "-qm", "add gitlink") + // Move the embedded repo's HEAD so the parent reports ` M sub`. + if err := os.WriteFile(filepath.Join(sub, "y.txt"), []byte("two\n"), 0o644); err != nil { + t.Fatal(err) + } + testGit(t, sub, "add", "-A") + testGit(t, sub, "commit", "-qm", "sub two") + + path, found, err := scanWorktreeForSecret(repo) + if err != nil { + t.Fatalf("a dirty gitlink wedged the gate: %v", err) + } + if found { + t.Fatalf("no secret was seeded, but the scan blocked on %q", path) + } +} + +// MAJOR: the untracked-directory branch must scan exactly what `git add -A` would +// stage. A gitignored file inside the directory is never committed, so scanning it +// blocks the sync on content that cannot leak — and ferry's own init writes an +// unanchored `local/` pattern, so this fires on ordinary local scratch space. +// The positive control in the same directory proves the branch still gates a file +// that WOULD be staged. +func TestScanWorktreeSkipsIgnoredFilesInUntrackedDirectory(t *testing.T) { + repo := r9Repo(t) + if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte("local/\n"), 0o644); err != nil { + t.Fatal(err) + } + testGit(t, repo, "add", ".gitignore") + testGit(t, repo, "commit", "-qm", "ignore local/") + + // A wholly-untracked directory: one stageable clean file, one IGNORED file + // carrying a fake credential. + if err := os.MkdirAll(filepath.Join(repo, "stuff", "local"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "stuff", "clean.txt"), []byte("nothing here\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "stuff", "local", "env.sh"), []byte(fakeRound9AWSKey), 0o644); err != nil { + t.Fatal(err) + } + + if path, found, err := scanWorktreeForSecret(repo); err != nil { + t.Fatalf("scanWorktreeForSecret errored on an untracked directory: %v", err) + } else if found { + t.Fatalf("an IGNORED file (%q) that `git add -A` would never stage blocked the sync", path) + } + + // Positive control: the same secret in a STAGEABLE file under the same + // directory must still block. + if err := os.WriteFile(filepath.Join(repo, "stuff", "creds.sh"), []byte(fakeRound9AWSKey), 0o644); err != nil { + t.Fatal(err) + } + path, found, err := scanWorktreeForSecret(repo) + if err != nil { + t.Fatalf("scanWorktreeForSecret errored: %v", err) + } + if !found { + t.Fatal("a secret in a stageable file inside an untracked directory was NOT commit-gated") + } + if filepath.ToSlash(path) != "stuff/creds.sh" { + t.Errorf("blocked path = %q, want stuff/creds.sh", path) + } +} + +// MAJOR: git records a nested repository as a gitlink, so its `.git/**` never +// enters a commit. Walking into it made a nested repo's credential-bearing remote +// URL block the sync with advice the user cannot satisfy (the file is not theirs +// to clean). `git ls-files -o` reports the nested repo as a single directory +// entry, which the branch skips. +func TestScanWorktreeSkipsNestedRepoUnderUntrackedDirectory(t *testing.T) { + repo := r9Repo(t) + if err := os.MkdirAll(filepath.Join(repo, "stuff"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "stuff", "clean.txt"), []byte("nothing here\n"), 0o644); err != nil { + t.Fatal(err) + } + nested := filepath.Join(repo, "stuff", "nested") + testGit(t, repo, "init", "-q", "-b", "main", filepath.Join("stuff", "nested")) + // A remote URL with an embedded token: high-confidence content in a file that + // is never committed by the outer repo. + testGit(t, nested, "remote", "add", "origin", fakeRound9TokenURL) + + path, found, err := scanWorktreeForSecret(repo) + if err != nil { + t.Fatalf("scanWorktreeForSecret errored on an untracked directory holding a nested repo: %v", err) + } + if found { + t.Fatalf("a nested repository's own %q was scanned and blocked the sync — its contents are never committed", path) + } +} + +// The enumeration itself must FAIL CLOSED: a directory whose contents git cannot +// list is a directory we cannot scan, so sync must refuse rather than commit it +// unscanned. The `git` shim answers `ls-files` with a failure and execs the real +// git for everything else (the technique cmd/sync_stderr_test.go uses). +func TestScanWorktreeFailsClosedWhenUntrackedDirectoryCannotBeListed(t *testing.T) { + repo := r9Repo(t) + if err := os.MkdirAll(filepath.Join(repo, "stuff"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "stuff", "clean.txt"), []byte("nothing here\n"), 0o644); err != nil { + t.Fatal(err) + } + real, err := exec.LookPath("git") + if err != nil { + t.Skip("git not on PATH") + } + dir := t.TempDir() + script := "#!/bin/sh\nfor a in \"$@\"; do\n if [ \"$a\" = \"ls-files\" ]; then echo 'fatal: cannot list' >&2; exit 128; fi\ndone\nexec " + real + " \"$@\"\n" + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + if _, _, err := scanWorktreeForSecret(repo); err == nil { + t.Fatal("a failed untracked-directory enumeration was accepted; the gate must fail closed so sync refuses rather than committing unscanned files") + } +} + +// fakeRound9Key is a NON-FUNCTIONAL private-key header — the shape +// internal/secret flags High. NOT a real key. +const fakeRound9Key = "-----BEGIN OPENSSH PRIVATE KEY-----\n" + + "b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW\n" + + "-----END OPENSSH PRIVATE KEY-----\n" + +// fakeRound9AWSKey is a fake AWS access-key line, matching the fixture style the +// round-3 scan tests use. NOT a real credential. +const fakeRound9AWSKey = "export AWS_ACCESS_KEY_ID=AKIA1234567890ABCDEF\n" + +// fakeRound9TokenURL is a fake GitHub-token-bearing remote URL. NOT a real token. +const fakeRound9TokenURL = "https://user:ghp_0123456789abcdefghijABCDEFGHIJ012345@github.com/owner/repo.git" From 941f43c42da65f837fcdd1502a73b112ad1815c7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:00:48 +0000 Subject: [PATCH 2/8] fix: converge terminal captures over overlays and secret placeholders A shared-route terminal capture wrote the shared plist while a per-machine overlay from an earlier local capture kept winning both the drift comparison and apply, so the domain re-offered forever and the shared bytes did nothing; a shared accept now removes the superseded overlay and says so, leaving symlinked overlays untouched. A secret-routed capture wrote a placeholder that status and capture compared raw against the live export, reporting drift forever while apply rendered it in-sync; both compare sites now render placeholders through the secret store first (raw-compare fallback when the store or ref is unavailable), and the placeholder write round-trips byte-exactly. Assisted-by: Claude:claude-fable-5 --- cmd/capture.go | 64 +++++++++++++- cmd/status.go | 40 +++++++++ cmd/terminal_round9_test.go | 172 ++++++++++++++++++++++++++++++++++++ 3 files changed, 272 insertions(+), 4 deletions(-) create mode 100644 cmd/terminal_round9_test.go diff --git a/cmd/capture.go b/cmd/capture.go index 59d4ef3..6d60fac 100644 --- a/cmd/capture.go +++ b/cmd/capture.go @@ -1081,6 +1081,7 @@ func captureTerminalDomain(cc captureCtx, domain string) (wrote bool, offered bo return false, false, err } repoBytes, _ := os.ReadFile(compareSrc) + repoBytes = terminalRepoCompareBytes(cc.secretStore, repoBytes) if domain == "iterm2" { // Compare LIKE-FOR-LIKE: filter the repo side to the same allowlist so a repo // plist that happens to carry stale volatile keys never registers as drift. @@ -1112,10 +1113,9 @@ func captureTerminalDomain(cc captureCtx, domain string) (wrote bool, offered bo // Route the accepted whole domain. switch promptRoute(cc.in, cc.out) { case secret.RouteShared: - if err := writeRepoFile(cc.repoPath, repoDest, liveBlob); err != nil { + if err := acceptTerminalShared(cc.out, cc.repoPath, domain, prefID, liveBlob); err != nil { return false, true, err } - fmt.Fprintf(cc.out, " %s: captured -> shared (%s)\n", domain, relTo(cc.repoPath, repoDest)) return true, true, nil case secret.RouteLocal: // Guarantee the local layer is gitignored AT creation so the wholesale @@ -1158,14 +1158,70 @@ func captureBlockedTerminal(cc captureCtx, domain, prefID, repoDest string, live if err := cc.secretStore.Put(ref, string(liveBlob)); err != nil { return false, fmt.Errorf("write to secret store: %w", err) } - placeholder := secret.Placeholder(ref) - if err := writeRepoFile(cc.repoPath, repoDest, []byte(placeholder+"\n")); err != nil { + if err := writeRepoFile(cc.repoPath, repoDest, terminalPlaceholderBlob(ref)); err != nil { return false, err } fmt.Fprintf(cc.out, " %s: secret stored out-of-band in ~/.config/ferry/secrets-local; a placeholder was written to the repo\n", domain) return true, nil } +// acceptTerminalShared writes an accepted whole-domain export to the SHARED repo +// path and SUPERSEDES any per-machine local overlay for the same domain, so the +// shared accept actually converges. +// +// Every comparison of this domain (status's terminalLiveDiffers, capture's own +// compare, apply's terminalExportBlob) resolves LOCAL-WINS: while an overlay +// exists it shadows the shared copy. A shared capture written behind a surviving +// overlay therefore changed nothing observable — status reported drift forever, +// capture re-offered the domain forever, and apply kept importing the stale +// overlay. The accept is an explicit instruction to make THESE bytes the ones +// this machine carries, so the superseded overlay is removed and the removal is +// reported by path and reason. +// +// A removal failure is an ERROR, never a silent continue: leaving the overlay in +// place would keep the machine on the stale bytes while the capture reported +// success. An overlay that ferry REFUSES to read (symlinked/escaping — see +// regularRepoFile, which guards before it stats) never wins a comparison in the +// first place, so it is left exactly as found. +func acceptTerminalShared(out io.Writer, repo, domain, prefID string, blob []byte) error { + dest := terminalRepoDest(repo, domain, prefID) + if err := writeRepoFile(repo, dest, blob); err != nil { + return err + } + fmt.Fprintf(out, " %s: captured -> shared (%s)\n", domain, relTo(repo, dest)) + // Probe BOTH overlay names apply's terminalExportBlob accepts (.plist and + // the extensionless ), so no spelling of the overlay is left to shadow the + // shared copy on the next apply. + for _, cand := range []string{ + terminalLocalDest(repo, domain, prefID), + filepath.Join(repo, "local", domain, prefID), + } { + if !regularRepoFile(repo, cand) { + continue // absent, or refused: it never wins the comparison. + } + safe, err := safeRepoPath(repo, cand) + if err != nil { + return err + } + if err := os.Remove(safe); err != nil { + return fmt.Errorf("remove superseded local overlay %s: %w", relTo(repo, cand), err) + } + fmt.Fprintf(out, " %s: removed the local overlay %s (superseded by this shared capture; it would otherwise keep winning over the shared copy)\n", domain, relTo(repo, cand)) + } + return nil +} + +// terminalPlaceholderBlob is the repo file a secret-routed terminal capture writes +// in place of the exported preferences: the bare {{ferry.secret ...}} placeholder +// with NO added newline, so rendering it reproduces the stored export BYTE FOR +// BYTE. The trailing newline matters: the export already ends in one, and every +// comparison of the unfiltered (Apple Terminal) domain is a byte compare of the +// rendered repo blob against the live export — one extra byte and the domain +// reports drift forever. +func terminalPlaceholderBlob(ref string) []byte { + return []byte(secret.Placeholder(ref)) +} + // terminalRepoDest is the committed repo plist path apply READS for a terminal // domain — aligned with apply's buildTerminalDomain / terminalExportBlob: both // iTerm2 and Apple Terminal import the committed //.plist via diff --git a/cmd/status.go b/cmd/status.go index 1db8de8..5ab5648 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -14,6 +14,7 @@ import ( "github.com/REPPL/ferry/internal/deps" "github.com/REPPL/ferry/internal/dotfile" "github.com/REPPL/ferry/internal/platform" + "github.com/REPPL/ferry/internal/secret" "github.com/REPPL/ferry/internal/terminal" ) @@ -206,6 +207,7 @@ func terminalLiveDiffers(repo, domain string) bool { return false } repoBytes, _ := os.ReadFile(statusSrc) + repoBytes = terminalRepoCompareBytes(terminalSecretStore(), repoBytes) if domain == "iterm2" { // iTerm2 ONLY: compare LIKE-FOR-LIKE by reducing BOTH sides to the allowlisted // global keys. `defaults export` carries volatile machine state (window @@ -218,6 +220,44 @@ func terminalLiveDiffers(repo, domain string) bool { return string(repoBytes) != string(liveBlob) } +// terminalRepoCompareBytes renders the repo blob's {{ferry.secret ...}} +// placeholders EXACTLY as apply does (buildTerminalDomain -> renderSecrets) before +// the blob is compared with the live export. +// +// A secret-routed terminal capture stores the whole export out of band and leaves +// only a placeholder at the repo path. Comparing that placeholder RAW against the +// live plist can never be equal, so the domain reported drift forever — status +// re-flagged it, capture re-offered and re-gated it — while apply, which renders +// before importing, considered the very same domain in sync. Rendering first makes +// the two agree. +// +// CONSERVATIVE FALLBACK: a nil store, a render error, or a MISSING referenced +// secret returns the repo bytes UNCHANGED, i.e. the previous raw compare. The +// error case keeps its old behaviour rather than inventing a new one on a +// read-only path, and a missing secret is real divergence to surface (apply would +// SKIP the domain). A blob with no placeholders renders to itself, so every +// non-secret domain compares exactly as before. +func terminalRepoCompareBytes(store *secret.Store, repoBytes []byte) []byte { + if store == nil || len(repoBytes) == 0 { + return repoBytes + } + rendered, _, skip, err := renderSecrets(store, repoBytes) + if err != nil || skip { + return repoBytes + } + return rendered +} + +// terminalSecretStore opens the out-of-repo secret store for the read-only +// terminal comparison; a failure yields nil so the caller compares raw. +func terminalSecretStore() *secret.Store { + store, err := secret.Open() + if err != nil { + return nil + } + return store +} + // terminalRepoStatusSource resolves the repo plist status compares the live domain // against, mirroring apply's local-wins resolution: the per-machine local overlay // (local//.plist) when present, else the shared committed copy diff --git a/cmd/terminal_round9_test.go b/cmd/terminal_round9_test.go new file mode 100644 index 0000000..ae0bdd2 --- /dev/null +++ b/cmd/terminal_round9_test.go @@ -0,0 +1,172 @@ +package cmd + +// Ship-review round-9 regression tests for the terminal preference-domain +// convergence defects: +// +// - A3: a [s]hared capture behind an existing per-machine LOCAL overlay never +// converged. The shared route wrote /.plist, but every comparison +// (status and capture alike) resolves through terminalRepoStatusSource, where +// the overlay WINS — so the domain reported drift forever, capture re-offered +// it forever, and apply kept importing the stale overlay. +// - A4: a secret-routed capture writes a {{ferry.secret ...}} placeholder into +// the repo, but the comparison was a RAW byte compare against the live export. +// A placeholder never equals a plist, so the domain reported drift forever +// while apply (which renders placeholders) considered it in sync. +// +// The end-to-end capture/status paths are darwin-only (`defaults export`), so the +// coverage here is at the extracted helper seams both platforms compile. + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/REPPL/ferry/internal/secret" +) + +// syntheticExport is a stand-in for a `defaults export -` blob: opaque XML +// with a trailing newline, exactly the shape the live comparison sees. +const syntheticExport = "\n\n\n\tFoo\n\tbar\n\n\n" + +// A3: after a shared accept, the shared copy holds the new bytes AND the local +// overlay that would otherwise keep winning is gone — so the very next +// comparison resolves to the shared copy and the domain converges. +func TestAcceptTerminalShared_SupersedesLocalOverlay(t *testing.T) { + repo := t.TempDir() + const prefID = "com.apple.Terminal" + overlay := terminalLocalDest(repo, "terminal", prefID) + if err := os.MkdirAll(filepath.Dir(overlay), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(overlay, []byte("stale overlay\n"), 0o644); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := acceptTerminalShared(&out, repo, "terminal", prefID, []byte(syntheticExport)); err != nil { + t.Fatalf("acceptTerminalShared: %v", err) + } + + shared := terminalRepoDest(repo, "terminal", prefID) + got, err := os.ReadFile(shared) + if err != nil { + t.Fatalf("shared copy not written at %s: %v", shared, err) + } + if string(got) != syntheticExport { + t.Errorf("shared copy = %q, want the accepted export", got) + } + // The overlay must be GONE: while it exists it wins the comparison, so the + // domain would report drift forever and apply would re-import the stale bytes. + if _, serr := os.Lstat(overlay); !os.IsNotExist(serr) { + t.Errorf("the local overlay %s survived the shared accept (it would shadow the shared copy forever): stat err = %v", overlay, serr) + } + // And the resolution both status and capture use now points at the shared copy. + if src := terminalRepoStatusSource(repo, "terminal", prefID); src != shared { + t.Errorf("after the shared accept the compare source = %q, want the shared copy %q", src, shared) + } + // The removal is reported, naming the path and why. + msg := out.String() + if !strings.Contains(msg, "captured -> shared") { + t.Errorf("no shared-capture line in the output:\n%s", msg) + } + if !strings.Contains(msg, relTo(repo, overlay)) || !strings.Contains(msg, "superseded") { + t.Errorf("the removed overlay is not reported by path and reason:\n%s", msg) + } +} + +// A3: an overlay ferry refuses to read (a symlink out of the repo) never wins the +// comparison, so nothing is removed — and the refusal is not turned into an error. +func TestAcceptTerminalShared_LeavesSymlinkedOverlay(t *testing.T) { + repo := t.TempDir() + const prefID = "com.googlecode.iterm2" + outside := filepath.Join(t.TempDir(), "elsewhere.plist") + if err := os.WriteFile(outside, []byte("outside\n"), 0o644); err != nil { + t.Fatal(err) + } + overlay := terminalLocalDest(repo, "iterm2", prefID) + if err := os.MkdirAll(filepath.Dir(overlay), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, overlay); err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if err := acceptTerminalShared(&out, repo, "iterm2", prefID, []byte(syntheticExport)); err != nil { + t.Fatalf("acceptTerminalShared: %v", err) + } + if _, serr := os.Lstat(overlay); serr != nil { + t.Errorf("a refused (symlinked) overlay was removed; it never wins the comparison: %v", serr) + } + if _, serr := os.Lstat(outside); serr != nil { + t.Errorf("the symlink target outside the repo was touched: %v", serr) + } +} + +// A4: a placeholder-bearing repo blob whose secret IS present renders to the live +// export, so the comparison is equal and the domain converges. +func TestTerminalRepoCompareBytes_RendersPlaceholders(t *testing.T) { + store := secret.OpenAt(t.TempDir()) + const ref = "com.apple.Terminal.captured" + if err := store.Put(ref, syntheticExport); err != nil { + t.Fatal(err) + } + repoBytes := terminalPlaceholderBlob(ref) + + got := terminalRepoCompareBytes(store, repoBytes) + if string(got) != syntheticExport { + t.Errorf("rendered repo bytes = %q, want the live export %q (the domain reports drift forever otherwise)", got, syntheticExport) + } +} + +// A4: a MISSING referenced secret falls back to the raw compare — conservative, +// no behaviour change and no error on the read-only path. +func TestTerminalRepoCompareBytes_MissingRefFallsBackToRaw(t *testing.T) { + store := secret.OpenAt(t.TempDir()) + repoBytes := terminalPlaceholderBlob("com.apple.Terminal.captured") + + got := terminalRepoCompareBytes(store, repoBytes) + if string(got) != string(repoBytes) { + t.Errorf("missing ref: compare bytes = %q, want the raw repo bytes %q", got, repoBytes) + } + if string(got) == syntheticExport { + t.Error("a missing secret rendered anyway") + } +} + +// A4: blobs with no placeholders, and a nil store, are passed through untouched — +// the existing comparison semantics are unchanged for every non-secret domain. +func TestTerminalRepoCompareBytes_PassThrough(t *testing.T) { + store := secret.OpenAt(t.TempDir()) + if got := terminalRepoCompareBytes(store, []byte(syntheticExport)); string(got) != syntheticExport { + t.Errorf("plain repo bytes were altered: %q", got) + } + if got := terminalRepoCompareBytes(nil, []byte(syntheticExport)); string(got) != syntheticExport { + t.Errorf("nil store: repo bytes were altered: %q", got) + } + if got := terminalRepoCompareBytes(store, nil); len(got) != 0 { + t.Errorf("absent repo copy rendered to %q, want empty", got) + } +} + +// A4: the placeholder blob a secret-routed capture writes must render back to the +// export BYTE-FOR-BYTE — an extra trailing newline is enough to keep the domain +// reporting drift forever on the unfiltered (Apple Terminal) side. +func TestTerminalPlaceholderBlob_RoundTripsExport(t *testing.T) { + store := secret.OpenAt(t.TempDir()) + const ref = "com.apple.Terminal.captured" + for _, live := range []string{syntheticExport, strings.TrimSuffix(syntheticExport, "\n")} { + if err := store.Put(ref, live); err != nil { + t.Fatal(err) + } + rendered, missing, skip, err := renderSecrets(store, terminalPlaceholderBlob(ref)) + if err != nil || skip { + t.Fatalf("render: skip=%v missing=%v err=%v", skip, missing, err) + } + if string(rendered) != live { + t.Errorf("placeholder blob rendered to %q, want the exported blob %q", rendered, live) + } + } +} From cfd2ec86eecf998f76c950c1b3c0efa900848fac Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:00:48 +0000 Subject: [PATCH 3/8] fix: parse npm globals from stdout and carry npm diagnostics in errors The dump parsed CombinedOutput, so npm's stderr made the documented non-zero-exit tolerance unreachable and any peer-dependency warning disabled the domain. The listing query now captures streams separately (optional SeparateRunner on the exec runner; plain fakes fall back unchanged) and a genuine failure splices npm's stderr into the error instead of a bare exit status. Assisted-by: Claude:claude-fable-5 --- internal/deps/npm.go | 15 +++++- internal/deps/npm_status_test.go | 92 +++++++++++++++++++++++++++++--- internal/deps/runner.go | 65 ++++++++++++++++++++-- 3 files changed, 159 insertions(+), 13 deletions(-) diff --git a/internal/deps/npm.go b/internal/deps/npm.go index 4f1614e..9f8b289 100644 --- a/internal/deps/npm.go +++ b/internal/deps/npm.go @@ -50,14 +50,25 @@ type npmLsOutput struct { // the global tree has peer-dependency warnings while still emitting valid JSON, so // a parseable body is used even when the runner reports an error; only an // unparseable body is a hard failure. +// +// That tolerance only holds because the JSON is parsed from STDOUT ALONE: npm +// writes `npm ERR!` lines to STDERR on every non-zero exit, so a combined buffer +// is never a single parseable JSON value and the tolerated branch would be dead +// code. The query therefore runs through SeparateRunner when the runner offers it +// (ExecRunner does). On the hard-failure path npm's own stderr is spliced into the +// error, matching the sibling manager error paths. func DumpNpmGlobals(runner CommandRunner) ([]string, error) { if runner == nil { return nil, fmt.Errorf("deps: nil CommandRunner") } - out, runErr := runner.Run(append([]string{npmBin}, npmGlobalsQuery...)...) + args := append([]string{npmBin}, npmGlobalsQuery...) + stdout, stderr, runErr := runQuery(runner, args) var parsed npmLsOutput - if err := json.Unmarshal([]byte(out), &parsed); err != nil { + if err := json.Unmarshal([]byte(stdout), &parsed); err != nil { if runErr != nil { + if diag := strings.TrimSpace(stderr); diag != "" { + return nil, fmt.Errorf("deps: npm ls -g: %w (%s)", runErr, diag) + } return nil, fmt.Errorf("deps: npm ls -g: %w", runErr) } return nil, fmt.Errorf("deps: parse npm ls -g output: %w", err) diff --git a/internal/deps/npm_status_test.go b/internal/deps/npm_status_test.go index d966d0e..9636bab 100644 --- a/internal/deps/npm_status_test.go +++ b/internal/deps/npm_status_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "strings" "testing" @@ -139,8 +140,16 @@ func TestDumpNpmGlobals_NamesOnlySortedExcludesNpm(t *testing.T) { } func TestDumpNpmGlobals_ToleratesNonZeroExitWithJSON(t *testing.T) { - // `npm ls` exits non-zero on peer-dep warnings while still emitting valid JSON. - r := &jsonErrRunner{body: `{"dependencies":{"typescript":{"version":"5.4.0"}}}`} + // `npm ls` exits non-zero on peer-dep warnings while still emitting valid JSON + // on STDOUT and its `npm ERR!` diagnostics on STDERR. jsonErrRunner models that + // REAL two-stream shape (its Run fuses the streams the way CombinedOutput does, + // exactly as production npm would look through the plain CommandRunner seam), so + // the tolerated branch is exercised against the contract the runner really has — + // not against a pre-split, JSON-only body that could never occur. + r := &jsonErrRunner{ + stdout: `{"dependencies":{"typescript":{"version":"5.4.0"}}}`, + stderr: "npm ERR! code ELSPROBLEMS\nnpm ERR! peer dep missing\n", + } names, err := DumpNpmGlobals(r) if err != nil { t.Fatalf("DumpNpmGlobals with non-zero exit + valid JSON: %v", err) @@ -150,6 +159,47 @@ func TestDumpNpmGlobals_ToleratesNonZeroExitWithJSON(t *testing.T) { } } +// TestDumpNpmGlobals_RealExecToleratesNpmErrNoiseOnStderr is the regression for +// the fused-stream defect: driven through the REAL ExecRunner against a stub +// `npm` on PATH, a peer-dep run (valid JSON on stdout, `npm ERR!` lines on +// stderr, exit 1) must still parse. A fake that hands back pre-split streams +// cannot detect this — only a real subprocess can. +func TestDumpNpmGlobals_RealExecToleratesNpmErrNoiseOnStderr(t *testing.T) { + stubNpm(t, "#!/bin/sh\n"+ + "printf '{\"dependencies\":{\"typescript\":{\"version\":\"5.4.0\"},\"npm\":{\"version\":\"11.0.0\"}}}\\n'\n"+ + "printf 'npm ERR! code ELSPROBLEMS\\nnpm ERR! missing: pyright@1.1.0, required by lib\\n' >&2\n"+ + "exit 1\n") + + names, err := DumpNpmGlobals(ExecRunner{}) + if err != nil { + t.Fatalf("DumpNpmGlobals against a peer-dep npm (valid JSON on stdout, npm ERR! on stderr, exit 1): %v", err) + } + if want := []string{"typescript"}; !reflect.DeepEqual(names, want) { + t.Errorf("names = %v, want %v", names, want) + } +} + +// TestDumpNpmGlobals_RealExecHardFailureKeepsNpmDiagnostics: when stdout is NOT +// parseable and npm exits non-zero, the failure is hard — and the returned error +// must splice in npm's own stderr diagnostics (matching install.go / status.go / +// dump.go), not swallow them behind a bare "exit status 1". +func TestDumpNpmGlobals_RealExecHardFailureKeepsNpmDiagnostics(t *testing.T) { + stubNpm(t, "#!/bin/sh\n"+ + "printf 'not json at all\\n'\n"+ + "printf 'npm ERR! code ENOENT\\nnpm ERR! enoent ENOENT: no such file or directory\\n' >&2\n"+ + "exit 1\n") + + _, err := DumpNpmGlobals(ExecRunner{}) + if err == nil { + t.Fatalf("DumpNpmGlobals with unparseable output + non-zero exit: want error, got nil") + } + for _, want := range []string{"npm ls -g", "npm ERR! code ENOENT", "no such file or directory"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not carry npm's diagnostics (%q)", err, want) + } + } +} + func TestReDumpNpmGlobals_WritesSortedList(t *testing.T) { depsDir := t.TempDir() r := newFakeRunner() @@ -317,10 +367,40 @@ func TestValidateBrewfileDirective_StillRefusesNpm(t *testing.T) { // --- helpers ---------------------------------------------------------------- -// jsonErrRunner returns a fixed body together with a non-nil error, modelling -// `npm ls` exiting non-zero (peer-dep warnings) while still emitting valid JSON. -type jsonErrRunner struct{ body string } +// jsonErrRunner models `npm ls` exiting non-zero (peer-dep warnings) while still +// emitting valid JSON: the JSON goes to STDOUT and the `npm ERR!` diagnostics to +// STDERR — the real two-stream shape. Run fuses them the way CombinedOutput does +// (so a caller on the plain CommandRunner seam sees exactly what production would), +// while RunSeparate serves them apart, as ExecRunner now does. +type jsonErrRunner struct{ stdout, stderr string } func (j *jsonErrRunner) Run(_ ...string) (string, error) { - return j.body, errors.New("npm ls: exit status 1") + return j.stdout + j.stderr, errors.New("npm ls: exit status 1") +} + +func (j *jsonErrRunner) RunSeparate(_ ...string) (string, string, error) { + return j.stdout, j.stderr, errors.New("npm ls: exit status 1") +} + +// stubNpm puts an executable `npm` stub running script first on PATH for the +// duration of the test, so ExecRunner (which resolves npm through PATH) spawns a +// REAL subprocess with fully separate streams. Skipped where a POSIX shell stub +// cannot run. +func stubNpm(t *testing.T, script string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell stub on PATH is POSIX-only") + } + if _, err := os.Stat("/bin/sh"); err != nil { + t.Skip("/bin/sh not available for the npm stub") + } + dir := t.TempDir() + stub := filepath.Join(dir, npmBin) + if err := os.WriteFile(stub, []byte(script), 0o755); err != nil { + t.Fatalf("write npm stub: %v", err) + } + if err := os.Chmod(stub, 0o755); err != nil { + t.Fatalf("chmod npm stub: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) } diff --git a/internal/deps/runner.go b/internal/deps/runner.go index 1884950..83243f8 100644 --- a/internal/deps/runner.go +++ b/internal/deps/runner.go @@ -1,6 +1,7 @@ package deps import ( + "bytes" "errors" "fmt" "os" @@ -37,20 +38,74 @@ type ExecRunner struct{} // NOT a root rail (Homebrew refuses to run as root) and stays $PATH-resolved so // the eval harness can shadow it with a stub. func (ExecRunner) Run(args ...string) (string, error) { + cmd, err := managerCommand(args) + if err != nil { + return "", err + } + out, cmdErr := cmd.CombinedOutput() + return string(out), cmdErr +} + +// SeparateRunner is the OPTIONAL capability a CommandRunner may also implement: +// running a command with stdout and stderr captured SEPARATELY. It is optional on +// purpose — CommandRunner itself stays a one-method interface so every existing +// fake (in this package and in cmd/) keeps compiling — and callers that need +// unfused streams type-assert for it and fall back to Run. +// +// It exists because combined output is WRONG for machine-parsed manager output: +// `npm ls -g --json` exits non-zero on a peer-dependency problem while still +// emitting valid JSON on stdout, but it writes `npm ERR!` lines to stderr at the +// same time — fused, the buffer is never a single parseable JSON value, so the +// tolerated "non-zero exit with usable JSON" path could never be taken. This is +// the deps-rail counterpart of the same split in cmd's hardened git helper. +type SeparateRunner interface { + RunSeparate(args ...string) (stdout, stderr string, err error) +} + +// RunSeparate executes args[0] with the remaining args and returns stdout and +// stderr as SEPARATE strings. Program resolution is identical to Run (root-rail +// managers through the sanitized seam, everything else through PATH); only the +// stream capture differs. +func (ExecRunner) RunSeparate(args ...string) (string, string, error) { + cmd, err := managerCommand(args) + if err != nil { + return "", "", err + } + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + cmdErr := cmd.Run() + return stdout.String(), stderr.String(), cmdErr +} + +// runQuery runs a read-only manager query with stdout and stderr apart. A runner +// that implements SeparateRunner serves them genuinely separated; any other +// CommandRunner (a test fake) has only its one fused buffer, which is returned as +// BOTH streams — the pre-existing behaviour for such runners, so no fake changes +// meaning, while production (ExecRunner) gets the real split. +func runQuery(runner CommandRunner, args []string) (stdout, stderr string, err error) { + if sr, ok := runner.(SeparateRunner); ok { + return sr.RunSeparate(args...) + } + out, runErr := runner.Run(args...) + return out, out, runErr +} + +// managerCommand builds the *exec.Cmd for a manager invocation, applying the +// root-rail resolution rule shared by Run and RunSeparate. +func managerCommand(args []string) (*exec.Cmd, error) { if len(args) == 0 { - return "", errors.New("deps: Run called with no command") + return nil, errors.New("deps: Run called with no command") } prog := args[0] if isRootRailManager(prog) { resolved, err := lookManager(prog) if err != nil { - return "", err + return nil, err } prog = resolved } - cmd := exec.Command(prog, args[1:]...) //nolint:gosec // prog is a fixed manager name (root rail: sanitized-absolute), not user input - out, err := cmd.CombinedOutput() - return string(out), err + return exec.Command(prog, args[1:]...), nil //nolint:gosec // prog is a fixed manager name (root rail: sanitized-absolute), not user input } // lookManager resolves a ROOT-RAIL manager binary (apt-get / dpkg-query) to a From 9b10fb2e8e0f09b33886e5d77821cd1bdf694aa0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:00:57 +0000 Subject: [PATCH 4/8] fix: predict the empty-over-substantial refusal and report honestly The data-loss guard that aborts apply when a near-empty repo file would replace a substantial live file was invisible to the preview, which showed a plain would-update; the plan now renders such items as would refuse and the abort path states that the changes reported earlier in the run were rolled back (the abort policy itself is deliberate and unchanged). The guided walkthrough tells the user that confirming a conflict does not overwrite it, a clean skip-always target is silent and counted in sync, and the receive planner accumulates lock releases so a second union-merge item cannot leak a lock. Assisted-by: Claude:claude-fable-5 --- cmd/apply.go | 102 +++++++++++++++---- cmd/apply_guard_preview_test.go | 167 +++++++++++++++++++++++++++++++ cmd/guided_apply.go | 28 +++++- cmd/guided_apply_test.go | 123 +++++++++++++++++++++++ evals/harness.go | 9 +- internal/dotfile/apply.go | 40 ++++++-- internal/dotfile/dotfile_test.go | 53 ++++++++++ internal/work/receive.go | 32 +++++- internal/work/receive_test.go | 36 +++++++ 9 files changed, 557 insertions(+), 33 deletions(-) create mode 100644 cmd/apply_guard_preview_test.go diff --git a/cmd/apply.go b/cmd/apply.go index 3ee9aee..cb0c682 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -727,8 +727,10 @@ func applyPlan(ctx *cmdContext, force bool, gopts guidedOpts, in *bufio.Reader, // we must roll THIS run back INLINE before returning — not wait for the next // apply's RollbackIncomplete (that net is only for a real crash, where no // in-process handler can run). mutate performs the whole per-target apply and - // returns its error; on a non-nil error we roll the in-progress run back here. - if err := mutate(eng, b, backupResource, commitRun, dec.toApply, force, out); err != nil { + // returns its error plus the number of changes it already REPORTED to the user + // before failing — those lines are undone by the rollback below, so they must + // be retracted in words. + if applied, err := mutate(eng, b, backupResource, commitRun, dec.toApply, force, out); err != nil { // Roll back the current run's recorded changes immediately so a failed apply // leaves the machine in its pre-apply state (files restored, terminal // resources re-imported/deleted to their captured baseline) rather than half @@ -740,6 +742,13 @@ func applyPlan(ctx *cmdContext, force bool, gopts guidedOpts, in *bufio.Reader, // machine may be left partially mutated. Surface BOTH errors loudly. return fmt.Errorf("apply failed (%v); inline rollback also failed (machine may be partially applied): %w", err, rbErr) } + // The rollback SUCCEEDED, so every "created"/"updated" line printed above is + // now a lie about the machine's state. Retract them explicitly — scrollback + // must never be left asserting writes this run reverted (the data-loss guard + // aborting mid-plan is the common way to get here). + if notice := rolledBackNotice(applied); notice != "" { + fmt.Fprintln(out, notice) + } return err } @@ -761,10 +770,16 @@ func applyPlan(ctx *cmdContext, force bool, gopts guidedOpts, in *bufio.Reader, // commit. It returns the first in-process error; the caller rolls the open run back // inline on any such error, so mutate itself just returns — it never leaves the run // committed on failure. The happy path commits normally and is idempotent. -func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain string) error, commitRun func() error, plan []planItem, force bool, out io.Writer) error { +// +// applied counts the changes mutate has REPORTED as written so far (a created or +// updated file, an applied preference domain) — not noops, skips, or conflicts, +// which assert no change. It is returned on the error path too, so the caller can +// retract exactly those lines once the inline rollback has reverted them +// (rolledBackNotice). +func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain string) error, commitRun func() error, plan []planItem, force bool, out io.Writer) (applied int, err error) { lastApplied, err := dotfile.OpenStore() if err != nil { - return fmt.Errorf("open last-applied store: %w", err) + return applied, fmt.Errorf("open last-applied store: %w", err) } var conflicts []string @@ -795,7 +810,7 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s // is what an apply failure rolls back to via d.Restore. eng.Register(d) if err := backupResource(d.Domain()); err != nil { - return fmt.Errorf("back up %s preference domain: %w", it.domain, err) + return applied, fmt.Errorf("back up %s preference domain: %w", it.domain, err) } // Secure the inline-rollback state BEFORE mutating, and FAIL CLOSED: we // never run terminal.Apply (the mutation) without a valid pre-mutation @@ -812,7 +827,7 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s fmt.Fprintf(out, " %-22s skipped (macOS only)\n", it.domain) continue } - return fmt.Errorf("capture %s preference domain before mutating: %w", it.domain, blobErr) + return applied, fmt.Errorf("capture %s preference domain before mutating: %w", it.domain, blobErr) } res := terminal.Apply(d) if res.Skipped && errors.Is(res.Err, terminal.ErrNotDarwin) { @@ -839,10 +854,11 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s // other target this run touched too); RollbackIncomplete remains a third // line of defence for a real crash. if rbErr := d.Restore(capturedBlob, capturedAbsent); rbErr != nil { - return fmt.Errorf("apply %s preference domain failed (%v); inline rollback also failed: %w", it.domain, res.Err, rbErr) + return applied, fmt.Errorf("apply %s preference domain failed (%v); inline rollback also failed: %w", it.domain, res.Err, rbErr) } - return fmt.Errorf("apply %s preference domain: %w", it.domain, res.Err) + return applied, fmt.Errorf("apply %s preference domain: %w", it.domain, res.Err) } + applied++ fmt.Fprintf(out, " %-22s preference domain applied\n", it.domain) if res.Note != "" { fmt.Fprintf(out, " %-22s note: %s\n", "", res.Note) @@ -895,7 +911,7 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s } // The empty-over-substantial data-loss guard aborts the run (rolled // back inline by the caller); a conflict is reported and skipped above. - return err + return applied, err } // --force pushed an empty/near-empty repo source OVER a substantial live // file. The overwrite proceeded (documented force semantics), but WARN, @@ -918,19 +934,24 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s // hash, never the plaintext bytes. deferred = append(deferred, res) it.action = string(res.Action) + // Only a REAL write counts toward the rollback retraction: a noop/skipped + // line asserts no change an abort would have to take back. + if res.Action == dotfile.ActionCreated || res.Action == dotfile.ActionUpdated { + applied++ + } fmt.Fprintf(out, " %-22s %s\n", it.domain, res.Action) } } if err := commitRun(); err != nil { - return fmt.Errorf("commit apply run: %w", err) + return applied, fmt.Errorf("commit apply run: %w", err) } // Persist deferred last-applied ONLY after the journal commit succeeds, so a // crash/commit-error between the file write and here can never leave // last-applied ahead of a rolled-back file (Codex#3). CommitLastApplied // ignores results with no PendingHash (noop/skipped), so passing all is safe. if err := dotfile.CommitLastApplied(deferred, lastApplied); err != nil { - return fmt.Errorf("commit last-applied: %w", err) + return applied, fmt.Errorf("commit last-applied: %w", err) } // Union this plan's agents targets into the persisted record (cumulative — // entries are never removed) so `ferry restore agents` can resolve the @@ -939,16 +960,28 @@ func mutate(eng *backup.Engine, b dotfile.Backuper, backupResource func(domain s if len(agentsTargets) > 0 { stateDir, err := paths.StateDir() if err != nil { - return fmt.Errorf("record agents targets: %w", err) + return applied, fmt.Errorf("record agents targets: %w", err) } if err := agents.RecordTargets(stateDir, agentsTargets); err != nil { - return fmt.Errorf("record agents targets: %w", err) + return applied, fmt.Errorf("record agents targets: %w", err) } } if len(conflicts) > 0 { fmt.Fprintf(out, "%d conflict(s) left unchanged: %s\n", len(conflicts), strings.Join(conflicts, ", ")) } - return nil + return applied, nil +} + +// rolledBackNotice renders the retraction an aborted apply owes the user: mutate +// printed a "created"/"updated" line for each change it made before the failure, +// and the caller's inline rollback then reverted every one of them, so those lines +// no longer describe the machine. Returns "" when nothing had been reported yet +// (there is nothing to take back). +func rolledBackNotice(applied int) string { + if applied <= 0 { + return "" + } + return fmt.Sprintf("apply aborted: the %d change(s) reported above were rolled back — this machine is unchanged", applied) } // fileConflictMessage renders the per-domain CONFLICT report line body for a @@ -1076,7 +1109,7 @@ func printPlan(out io.Writer, plan []planItem) { } colour := stateColourer(out) - var create, update, conflict int + var create, update, conflict, refuse int fmt.Fprintln(out, "ferry would apply:") for _, it := range plan { @@ -1106,6 +1139,19 @@ func printPlan(out io.Writer, plan []planItem) { } } case kindFile: + // Preview fidelity for the empty-over-substantial data-loss guard: apply + // ABORTS this write (the guard refuses to let an empty/near-empty repo + // source erase a substantial live file), so the preview must not promise + // a "would update" that cannot happen. The prediction reads the SAME + // predicate the guard enforces, so the two can never drift. It is + // domain-independent (the guard is), hence one check ahead of the + // per-domain arms. Read-only: it stats/reads the live file, writes + // nothing. + if !it.skip && emptyOverSubstantialPreview(it) { + refuse++ + fmt.Fprintf(out, " %-22s %s (empty repo source over a substantial live file; apply would abort — re-run with `--force` to overwrite anyway)\n", it.domain, colour(colRed, "would refuse")) + continue + } // Converged FileDomain rendering (fn-5): all file targets share the // three-way state lines; the owning domain (it.fileDomain) selects the // locally-drifted / conflict guidance that used to be keyed on the kind — @@ -1188,15 +1234,34 @@ func printPlan(out io.Writer, plan []planItem) { } // One-line summary footer, only counting the actionable states. - if summary := planSummary(create, update, conflict); summary != "" { + if summary := planSummary(create, update, refuse, conflict); summary != "" { fmt.Fprintf(out, "\n%s\n", summary) } } +// emptyOverSubstantialPreview reports whether apply's empty-over-substantial +// data-loss guard would REFUSE this item's write, so the preview can render the +// refusal instead of a "would update" that aborts. It only asks the question for +// the states that actually reach the write (a missing live file has nothing to +// erase; a locally-drifted target is skipped; a conflict is already refused and +// reported as such). The verdict comes from dotfile.WouldRefuseEmptyOverSubstantial +// — the SAME predicate the write-time guard enforces — so the preview cannot +// drift from the thresholds. +func emptyOverSubstantialPreview(it planItem) bool { + if it.kind != kindFile || it.state != dotfile.StateRepoAhead { + return false + } + _, dangerous := dotfile.WouldRefuseEmptyOverSubstantial(it.target, it.content) + return dangerous +} + // planSummary renders a compact "N would create, M would update, K conflict" // footer from the counted states, omitting zero categories. Returns "" when there // is nothing actionable to summarise (all-clean plans are handled earlier). -func planSummary(create, update, conflict int) string { +// refuse counts the targets the empty-over-substantial data-loss guard would +// abort on — reported after the conflicts, since it is the same "nothing will be +// written" family. +func planSummary(create, update, refuse, conflict int) string { var parts []string if create > 0 { parts = append(parts, fmt.Sprintf("%d would create", create)) @@ -1207,6 +1272,9 @@ func planSummary(create, update, conflict int) string { if conflict > 0 { parts = append(parts, fmt.Sprintf("%d conflict", conflict)) } + if refuse > 0 { + parts = append(parts, fmt.Sprintf("%d would refuse", refuse)) + } return strings.Join(parts, ", ") } diff --git a/cmd/apply_guard_preview_test.go b/cmd/apply_guard_preview_test.go new file mode 100644 index 0000000..a4e03ae --- /dev/null +++ b/cmd/apply_guard_preview_test.go @@ -0,0 +1,167 @@ +package cmd + +// A8 (preview fidelity + abort honesty): the empty-over-substantial data-loss +// guard aborts apply at WRITE time, but the preview used to render the same +// target as a plain "would update" — promising a change that cannot happen. The +// preview now predicts the refusal from the SAME predicate the guard enforces +// (dotfile.WouldRefuseEmptyOverSubstantial), and an aborted run says out loud +// that the changes it already reported were rolled back. + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/REPPL/ferry/internal/dotfile" +) + +// substantialLive is a live file well over the guard's 64-significant-byte bar. +const substantialLive = "export PATH=/usr/local/bin:$PATH\nalias gs='git status'\nalias gd='git diff'\n" + +// seedLive writes content to a fresh temp home path and returns it. +func seedLive(t *testing.T, name, content string) string { + t.Helper() + home := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(home, []byte(content), 0o644); err != nil { + t.Fatalf("seed live %s: %v", name, err) + } + return home +} + +// TestPrintPlanPredictsEmptyOverSubstantialRefusal pins the preview: a repo +// source that is empty/near-empty over a substantial live file must be rendered +// as a refusal, never as a "would update" apply will abort on. Every file domain +// shares the prediction (dotfiles, agents, and the repo-authoritative domains). +func TestPrintPlanPredictsEmptyOverSubstantialRefusal(t *testing.T) { + for _, domain := range []string{"dotfiles", "agents", "terminals"} { + t.Run(domain, func(t *testing.T) { + home := seedLive(t, ".zshrc", substantialLive) + plan := []planItem{{ + kind: kindFile, + fileDomain: domain, + domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: home}, + content: []byte("# nothing here\n"), + state: dotfile.StateRepoAhead, + }} + var out bytes.Buffer + printPlan(&out, plan) + got := out.String() + if !strings.Contains(got, "would refuse") { + t.Errorf("preview must predict the guard's refusal:\n%s", got) + } + if strings.Contains(got, "would update") { + t.Errorf("preview must NOT promise an update the guard aborts:\n%s", got) + } + if !strings.Contains(got, "1 would refuse") { + t.Errorf("summary must count the predicted refusal:\n%s", got) + } + }) + } +} + +// TestPrintPlanKeepsWouldUpdateForRealContent guards the negative side: an +// ordinary repo-ahead update with real content is untouched by the prediction. +func TestPrintPlanKeepsWouldUpdateForRealContent(t *testing.T) { + home := seedLive(t, ".zshrc", substantialLive) + plan := []planItem{{ + kind: kindFile, + fileDomain: "dotfiles", + domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: home}, + content: []byte(substantialLive + "alias gl='git log'\n"), + state: dotfile.StateRepoAhead, + }} + var out bytes.Buffer + printPlan(&out, plan) + got := out.String() + if !strings.Contains(got, "would update") { + t.Errorf("a real-content repo-ahead update must still read as an update:\n%s", got) + } + if strings.Contains(got, "would refuse") { + t.Errorf("a real-content update must never be predicted as a refusal:\n%s", got) + } +} + +// TestPlanSummaryCountsRefusals pins the footer's new category and its ordering. +func TestPlanSummaryCountsRefusals(t *testing.T) { + t.Parallel() + if got := planSummary(1, 2, 3, 4); got != "1 would create, 2 would update, 4 conflict, 3 would refuse" { + t.Errorf("planSummary = %q", got) + } + if got := planSummary(0, 0, 0, 0); got != "" { + t.Errorf("an empty summary must stay empty, got %q", got) + } + if got := planSummary(0, 0, 1, 0); got != "1 would refuse" { + t.Errorf("planSummary = %q", got) + } +} + +// TestRolledBackNoticeIsHonestAboutRevertedWork pins the abort-honesty line: when +// the guard (or any in-process error) aborts a run and the inline rollback +// succeeds, scrollback must not be left asserting writes that were reverted. +func TestRolledBackNoticeIsHonestAboutRevertedWork(t *testing.T) { + t.Parallel() + if got := rolledBackNotice(0); got != "" { + t.Errorf("nothing reported means nothing to retract, got %q", got) + } + got := rolledBackNotice(3) + for _, want := range []string{"3 change(s)", "rolled back"} { + if !strings.Contains(got, want) { + t.Errorf("rollback notice must contain %q: %q", want, got) + } + } +} + +// TestMutateCountsReportedWritesWhenTheGuardAborts pins the count the rollback +// retraction is built on: the data-loss guard aborts the run mid-plan, and mutate +// reports how many changes it had ALREADY printed as written (the ones the +// caller's inline rollback then reverts). Noops are not counted — they assert no +// change to take back. +func TestMutateCountsReportedWritesWhenTheGuardAborts(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + // 1: a fresh create (counts). 2: already in sync (noop, does not count). + // 3: an empty repo source over a substantial live file — the guard aborts here. + created := filepath.Join(home, ".gitconfig") + clean := filepath.Join(home, ".tmux.conf") + if err := os.WriteFile(clean, []byte("set -g mouse on\n"), 0o644); err != nil { + t.Fatalf("seed clean: %v", err) + } + guarded := filepath.Join(home, ".zshrc") + if err := os.WriteFile(guarded, []byte(substantialLive), 0o644); err != nil { + t.Fatalf("seed guarded: %v", err) + } + + plan := []planItem{ + {kind: kindFile, fileDomain: "dotfiles", domain: ".gitconfig", + target: dotfile.Target{Name: "gitconfig", Home: created}, content: []byte("[user]\n\tname = a\n")}, + {kind: kindFile, fileDomain: "dotfiles", domain: ".tmux.conf", + target: dotfile.Target{Name: "tmux.conf", Home: clean}, content: []byte("set -g mouse on\n")}, + {kind: kindFile, fileDomain: "dotfiles", domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: guarded}, content: []byte("")}, + } + + b := backuperFunc(func(dest string, content []byte, perm os.FileMode) error { + return os.WriteFile(dest, content, perm) + }) + committed := false + var out bytes.Buffer + applied, err := mutate(nil, b, func(string) error { return nil }, func() error { committed = true; return nil }, plan, false, &out) + if err == nil { + t.Fatalf("the data-loss guard must abort the run; out:\n%s", out.String()) + } + if committed { + t.Errorf("an aborted run must never commit the journal") + } + if applied != 1 { + t.Errorf("applied = %d, want 1 (the create; the noop asserts no change)\n%s", applied, out.String()) + } + // The guarded live file is untouched — the guard writes nothing. + if got, _ := os.ReadFile(guarded); string(got) != substantialLive { + t.Errorf("the guard must leave the live file byte-identical, got %q", got) + } +} diff --git a/cmd/guided_apply.go b/cmd/guided_apply.go index 82e3025..92e2bf4 100644 --- a/cmd/guided_apply.go +++ b/cmd/guided_apply.go @@ -173,6 +173,16 @@ func decideGuided(ctx *cmdContext, plan []planItem, force bool, gopts guidedOpts for _, it := range plan { if isGuidedKind(it.kind) && skipSet[itemKey(it)] { + // Only announce the exclusion when there is actually something to + // exclude. A skip-always target that ALREADY matches the repo has no + // pending work: reporting "skipped" every run is noise, and leaving it + // out of cleanCount made an otherwise in-sync machine claim "0 target(s) + // already match". It stays out of toApply either way — an excluded + // target is never written, and never adopted into last-applied. + if !planItemPending(it) { + res.cleanCount++ + continue + } fmt.Fprintf(out, " %-22s skipped (skip-always on this machine; delete it from %s to re-enable)\n", it.domain, skipAlwaysRel) continue } @@ -298,6 +308,20 @@ func groupRisky(risky []planItem) []riskyGroup { return groups } +// noOverwriteSuffix flags the risky items that confirming will NOT write. The +// walkthrough prompt reads "Apply all N change(s)…", but a CONFLICT (edited +// locally AND in the repo) is refused by the apply core without --force: it is +// reported and left unchanged. The walkthrough still carries conflicts — they hold +// the only diff view of the divergence — so the listing says out loud what "yes" +// does here, rather than letting the prompt imply an overwrite. Empty for every +// other item (those really are applied on "yes"). +func noOverwriteSuffix(it planItem) string { + if it.kind == kindFile && it.state == dotfile.StateConflict { + return ` — "yes" will NOT overwrite this item (` + "`ferry apply --force`" + ` does)` + } + return "" +} + // walkRisky is the interactive walkthrough over the risky changes, grouped by // domain. For each group the user confirms the whole group ("yes"), skips it this // run (anything else), or drills into per-item review ("details"). Every decision @@ -310,7 +334,7 @@ func walkRisky(in *bufio.Reader, out io.Writer, risky []planItem, store *dotfile g := groups[gi] fmt.Fprintf(out, "\n%s — %d change(s) need review:\n", g.name, len(g.items)) for _, it := range g.items { - fmt.Fprintf(out, " - %-22s %s\n", it.domain, it.riskReason) + fmt.Fprintf(out, " - %-22s %s%s\n", it.domain, it.riskReason, noOverwriteSuffix(it)) } ans, eof := readGuidedLine(in, out, fmt.Sprintf( "Apply all %d change(s) in %s? Type \"yes\" to apply, \"details\" to review each, anything else to skip this run: ", len(g.items), g.name)) @@ -381,7 +405,7 @@ func remainingItems(groups []riskyGroup) []planItem { // live — consuming the Foundation snapshot) followed by what apply would write // (live vs the repo content). func showItemDiff(out io.Writer, it planItem, store *dotfile.Store) { - fmt.Fprintf(out, "\n --- %s: %s ---\n", it.domain, it.riskReason) + fmt.Fprintf(out, "\n --- %s: %s%s ---\n", it.domain, it.riskReason, noOverwriteSuffix(it)) if it.secretRouted { fmt.Fprintln(out, " (secret-routed: diff hidden so no secret value is printed)") return diff --git a/cmd/guided_apply_test.go b/cmd/guided_apply_test.go index 781d325..fc33b8f 100644 --- a/cmd/guided_apply_test.go +++ b/cmd/guided_apply_test.go @@ -1,6 +1,11 @@ package cmd import ( + "bufio" + "bytes" + "os" + "path/filepath" + "strings" "testing" "github.com/REPPL/ferry/internal/dotfile" @@ -58,3 +63,121 @@ func TestAssessRisk(t *testing.T) { }) } } + +// --- skip-always: a clean target is not "skipped", it is already in sync (A10) --- + +// seedSkipAlways writes a repo-local skip-always file naming keys and returns the +// repo root, plus points $HOME at a throwaway dir (decideGuided opens the +// last-applied store there). +func seedSkipAlways(t *testing.T, keys ...string) *cmdContext { + t.Helper() + t.Setenv("HOME", t.TempDir()) + repo := t.TempDir() + if err := os.MkdirAll(filepath.Join(repo, "local"), 0o755); err != nil { + t.Fatalf("make local layer: %v", err) + } + body := strings.Join(keys, "\n") + "\n" + if err := os.WriteFile(filepath.Join(repo, "local", "skip-always.txt"), []byte(body), 0o644); err != nil { + t.Fatalf("seed skip-always: %v", err) + } + return &cmdContext{RepoPath: repo} +} + +// TestDecideGuidedCleanSkipAlwaysTargetIsSilentAndCounted pins A10: a skip-always +// target that already matches the repo has nothing to skip. Announcing "skipped +// (skip-always…)" every run is noise, and leaving it out of cleanCount makes an +// otherwise in-sync machine report "0 target(s) already match". +func TestDecideGuidedCleanSkipAlwaysTargetIsSilentAndCounted(t *testing.T) { + ctx := seedSkipAlways(t, "zshrc") + plan := []planItem{{ + kind: kindFile, fileDomain: "dotfiles", domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: filepath.Join(t.TempDir(), ".zshrc")}, + state: dotfile.StateClean, + }} + + var out bytes.Buffer + dec, err := decideGuided(ctx, plan, false, guidedOpts{}, bufio.NewReader(strings.NewReader("")), &out) + if err != nil { + t.Fatalf("decideGuided: %v", err) + } + if strings.Contains(out.String(), "skip-always") { + t.Errorf("a clean skip-always target must not announce a skip every run:\n%s", out.String()) + } + if dec.cleanCount != 1 { + t.Errorf("cleanCount = %d, want 1 (the clean skip-always target is in sync)", dec.cleanCount) + } + if !dec.nothingToDo { + t.Errorf("nothing pending anywhere: decideGuided must short-circuit as in-sync") + } + if len(dec.toApply) != 0 { + t.Errorf("a skip-always target must never be handed to mutate, got %d item(s)", len(dec.toApply)) + } +} + +// TestDecideGuidedPendingSkipAlwaysTargetStillReportsTheSkip is the other half: +// when the target IS pending, the skip line is the honest report of work the user +// excluded — it must still print, and the item must still never be applied. +func TestDecideGuidedPendingSkipAlwaysTargetStillReportsTheSkip(t *testing.T) { + ctx := seedSkipAlways(t, "zshrc") + plan := []planItem{{ + kind: kindFile, fileDomain: "dotfiles", domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: filepath.Join(t.TempDir(), ".zshrc")}, + state: dotfile.StateRepoAhead, + }} + + var out bytes.Buffer + dec, err := decideGuided(ctx, plan, false, guidedOpts{}, bufio.NewReader(strings.NewReader("")), &out) + if err != nil { + t.Fatalf("decideGuided: %v", err) + } + if !strings.Contains(out.String(), "skip-always") { + t.Errorf("a PENDING skip-always target must still be reported:\n%s", out.String()) + } + if dec.cleanCount != 0 { + t.Errorf("cleanCount = %d, want 0 (the target is pending, not in sync)", dec.cleanCount) + } + if len(dec.toApply) != 0 || len(dec.refused) != 0 { + t.Errorf("a skip-always target is neither applied nor refused: toApply=%d refused=%d", len(dec.toApply), len(dec.refused)) + } +} + +// --- walkthrough honesty: confirming a conflict does not overwrite it (B4) --- + +// TestWalkRiskyConflictListingSaysYesWillNotOverwrite pins B4: the group prompt +// offers to "Apply all N change(s)", but a conflict item is reported and left +// unchanged even when confirmed (apply refuses without --force). The listing must +// say so, so the prompt cannot be read as a promise to overwrite. +func TestWalkRiskyConflictListingSaysYesWillNotOverwrite(t *testing.T) { + store, err := dotfile.OpenStoreAtReadOnly(t.TempDir()) + if err != nil { + t.Fatalf("open store: %v", err) + } + risky := []planItem{ + { + kind: kindFile, fileDomain: "dotfiles", domain: ".zshrc", + target: dotfile.Target{Name: "zshrc", Home: filepath.Join(t.TempDir(), ".zshrc")}, + state: dotfile.StateConflict, risky: true, + riskReason: "conflict: edited locally AND in the repo (run `ferry capture` first, or `ferry apply --force` to overwrite)", + }, + { + kind: kindFile, fileDomain: "dotfiles", domain: ".gitconfig", + target: dotfile.Target{Name: "gitconfig", Home: filepath.Join(t.TempDir(), ".gitconfig")}, + state: dotfile.StateRepoAhead, risky: true, + riskReason: "would overwrite local changes (the live file differs from what ferry last deployed)", + }, + } + + var out bytes.Buffer + if _, _, _, err := walkRisky(bufio.NewReader(strings.NewReader("skip\n")), &out, risky, store); err != nil { + t.Fatalf("walkRisky: %v", err) + } + got := out.String() + for _, line := range strings.Split(got, "\n") { + if strings.Contains(line, ".zshrc") && !strings.Contains(line, "will NOT overwrite") { + t.Errorf("the conflict listing must say confirming will not overwrite it:\n%s", got) + } + if strings.Contains(line, ".gitconfig") && strings.Contains(line, "will NOT overwrite") { + t.Errorf("a non-conflict risky item must NOT carry the no-overwrite suffix:\n%s", got) + } + } +} diff --git a/evals/harness.go b/evals/harness.go index abbc66c..470c7bc 100644 --- a/evals/harness.go +++ b/evals/harness.go @@ -160,9 +160,12 @@ func (s *Sandbox) FerryWithInput(stdin string, args ...string) (stdoutStr, stder // ApplyConfirmed runs `ferry apply` (plus any extra args) through the v0.5.0 // guided walkthrough with confirmation piped on stdin, so a RISKY change (a // first-touch adoption / overwrite of a pre-existing file, a secret-routed -// deploy, a conflict) is confirmed rather than failing closed. Several "yes" -// lines are piped so multi-domain (dotfiles + agents) walkthroughs are all -// confirmed; extra lines are harmless. Use this wherever a test deliberately +// deploy) is applied rather than failing closed. A CONFLICT is the exception: +// confirming it only carries it past the risk gate — the apply core still +// refuses to overwrite an uncaptured local edit without --force, so a confirmed +// conflict is REPORTED and left unchanged. Several "yes" lines are piped so +// multi-domain (dotfiles + agents) walkthroughs are all confirmed; extra lines +// are harmless. Use this wherever a test deliberately // drives an overwrite/adoption that the risk gate now halts on; a plain // create-where-absent apply needs no confirmation and can still use Ferry. func (s *Sandbox) ApplyConfirmed(args ...string) (stdout, stderr string, exitCode int) { diff --git a/internal/dotfile/apply.go b/internal/dotfile/apply.go index 6ba5567..e267024 100644 --- a/internal/dotfile/apply.go +++ b/internal/dotfile/apply.go @@ -414,6 +414,32 @@ func StripFerryOverlayDirective(content []byte) []byte { return stripFerryOverlayDirective(content) } +// WouldRefuseEmptyOverSubstantial reports whether deploying desired onto t.Home +// is the empty-over-substantial data-loss transition the guard refuses without +// --force: the user's managed source (ferry's injected overlay directive stripped +// first) carries no significant bytes AND the live file carries at least +// substantialThreshold of them. liveSize is the live file's significant-byte +// count when dangerous is true (0 otherwise). +// +// It is the ONE predicate behind both the write-time refusal +// (guardEmptyOverSubstantial) and the read-only preview (`ferry diff` / +// `ferry status`, which render a "would refuse" line instead of promising an +// update that aborts). Exported so the preview shares this definition rather +// than re-deriving the thresholds and drifting from the guard. +func WouldRefuseEmptyOverSubstantial(t Target, desired []byte) (liveSize int, dangerous bool) { + if !isNearEmpty(stripFerryOverlayDirective(desired)) { + return 0, false // user's managed source has real content: not the dangerous transition. + } + live, err := os.ReadFile(t.Home) + if err != nil { + return 0, false // can't read live as a regular file: leave it to the deploy path. + } + if !isSubstantial(live) { + return 0, false // live file is itself trivial: nothing meaningful to lose. + } + return significantBytes(live), true +} + // guardEmptyOverSubstantial enforces the empty-over-substantial data-loss guard // for a target whose live file exists and is about to be overwritten. It judges // the desired content against the live file (t.Home): when the desired content @@ -432,22 +458,16 @@ func StripFerryOverlayDirective(content []byte) []byte { // the strength of ferry's OWN boilerplate. desired is the exact in-memory content // the apply would write, so the guard judges precisely the bytes at stake. func guardEmptyOverSubstantial(t Target, desired []byte, force, dryRun bool, res *Result) error { - if !isNearEmpty(stripFerryOverlayDirective(desired)) { - return nil // user's managed source has real content: not the dangerous transition. - } - live, err := os.ReadFile(t.Home) - if err != nil { - return nil // can't read live as a regular file: leave it to the deploy path. - } - if !isSubstantial(live) { - return nil // live file is itself trivial: nothing meaningful to lose. + liveSize, dangerous := WouldRefuseEmptyOverSubstantial(t, desired) + if !dangerous { + return nil } if !force { res.Action = ActionConflict return &EmptyOverSubstantialError{ Result: *res, Path: t.Home, - LiveSize: significantBytes(live), + LiveSize: liveSize, } } // --force: proceed, but flag the hazard so the caller warns. (dryRun never diff --git a/internal/dotfile/dotfile_test.go b/internal/dotfile/dotfile_test.go index 40c2b55..073b0fe 100644 --- a/internal/dotfile/dotfile_test.go +++ b/internal/dotfile/dotfile_test.go @@ -1288,3 +1288,56 @@ func TestStripFerryOverlayDirectivePrecision(t *testing.T) { t.Errorf("ferry's own injected block should strip to near-empty, got %q", got) } } + +// TestWouldRefuseEmptyOverSubstantial pins the EXPORTED preview predicate against +// the same thresholds the write-time guard enforces, so `ferry diff` can predict +// the refusal instead of promising a "would update" that aborts. It must agree +// with guardEmptyOverSubstantial on every axis: the ferry overlay directive is +// stripped before judging near-emptiness, a trivial live file is not worth +// guarding, and an absent live file is nothing to erase. +func TestWouldRefuseEmptyOverSubstantial(t *testing.T) { + substantial := "export PATH=/usr/local/bin:$PATH\nalias gs='git status'\nalias gd='git diff'\n" + if significantBytes([]byte(substantial)) < substantialThreshold { + t.Fatalf("test fixture is not substantial: %d bytes", significantBytes([]byte(substantial))) + } + + cases := []struct { + name string + live string // "" means: no live file at all + liveMiss bool + desired string + want bool + wantSize bool // expect a non-zero reported live size + }{ + {name: "empty repo source over substantial live", live: substantial, desired: "", want: true, wantSize: true}, + {name: "comments-only repo source over substantial live", live: substantial, desired: "# nothing here\n\n", want: true, wantSize: true}, + { + name: "ferry's own overlay block is stripped before judging", + live: substantial, + desired: "\n" + ferryOverlayMarker + "\n[ -f ~/.zshrc.local ] && source ~/.zshrc.local\n", + want: true, wantSize: true, + }, + {name: "real repo source is never refused", live: substantial, desired: substantial, want: false}, + {name: "trivial live file is nothing to lose", live: "# just a comment\n", desired: "", want: false}, + {name: "absent live file is nothing to erase", liveMiss: true, desired: "", want: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + home := filepath.Join(t.TempDir(), ".zshrc") + if !tc.liveMiss { + if err := os.WriteFile(home, []byte(tc.live), 0o644); err != nil { + t.Fatalf("seed live: %v", err) + } + } + tgt := Target{Name: "zshrc", Home: home} + size, got := WouldRefuseEmptyOverSubstantial(tgt, []byte(tc.desired)) + if got != tc.want { + t.Errorf("WouldRefuseEmptyOverSubstantial = %v, want %v", got, tc.want) + } + if tc.wantSize && size <= 0 { + t.Errorf("a predicted refusal must report the live significant-byte count, got %d", size) + } + }) + } +} diff --git a/internal/work/receive.go b/internal/work/receive.go index d4172fe..597633f 100644 --- a/internal/work/receive.go +++ b/internal/work/receive.go @@ -272,6 +272,10 @@ func planReceive(lc Locator, m *Manifest, contents map[string]cargoContent, stat baseline = state.Baseline.Files } + // Every advisory lock this plan takes, in acquisition order; unlock is kept in + // step with it so an early error return still releases whatever was taken. + var releases []func() + for _, mi := range m.Items { if !mi.Included { continue @@ -367,7 +371,13 @@ func planReceive(lc Locator, m *Manifest, contents map[string]cargoContent, stat if err != nil { return nil, nil, nil, unlock, err } - unlock = release + // ACCUMULATE, never overwrite: a plain `unlock = release` would drop + // the previous item's release, stranding its O_EXCL .lock file forever + // (no later receive could ever take that lock again). The registry + // carries one union-merge item today, so this is byte-identical now and + // correct the moment a second one is added. + releases = append(releases, release) + unlock = releaseAll(releases) writes = append(writes, trWrites...) } @@ -480,6 +490,26 @@ func hashDir(root string) (map[string]string, error) { return out, nil } +// releaseAll folds a set of acquired lock releases into ONE unlock func that runs +// them all, newest first (the order a stack of defers would unwind). It snapshots +// the slice, so a func returned mid-plan stays valid as later locks are appended. +// Returns nil when nothing was acquired — planReceive's callers test unlock for +// nil before deferring it. +func releaseAll(releases []func()) func() { + if len(releases) == 0 { + return nil + } + snapshot := make([]func(), len(releases)) + copy(snapshot, releases) + return func() { + for i := len(snapshot) - 1; i >= 0; i-- { + if snapshot[i] != nil { + snapshot[i]() + } + } + } +} + // acquireDirLock takes the advisory lock file in dir (creating dir first), // returning the release func. A held lock is a refusal, not a wait. func acquireDirLock(dir string) (func(), error) { diff --git a/internal/work/receive_test.go b/internal/work/receive_test.go index 2634363..826a2ce 100644 --- a/internal/work/receive_test.go +++ b/internal/work/receive_test.go @@ -466,3 +466,39 @@ func TestReceive_MidWriteFailureLeavesRevertibleState(t *testing.T) { t.Error("NEXT.md survived the revert of the partial receive") } } + +// TestReleaseAllRunsEveryReleaseInReverseOrder pins the fix for the lock-release +// leak in planReceive: the loop used to assign `unlock = release`, so a SECOND +// union-merge item would overwrite the first item's release and strand its +// O_EXCL .lock file forever (nothing would ever remove it, and every later +// receive would refuse). Releases are now accumulated and run together, newest +// first — the same order a stack of defers would unwind. +// +// The builtin registry carries exactly ONE union-merge item today +// (ItemTranscripts) and is not injectable, so two locks cannot be driven through +// planReceive itself; this pins the accumulator that makes the second one safe. +func TestReleaseAllRunsEveryReleaseInReverseOrder(t *testing.T) { + if releaseAll(nil) != nil { + t.Errorf("no acquired lock must yield a nil unlock (callers test for nil)") + } + + var order []string + first := func() { order = append(order, "first") } + second := func() { order = append(order, "second") } + + // A release captured BEFORE later acquisitions must still run: the accumulator + // is snapshotted, so growing the slice afterwards cannot drop or duplicate it. + unlockAfterOne := releaseAll([]func(){first}) + unlockAfterTwo := releaseAll([]func(){first, second}) + + unlockAfterTwo() + if got := strings.Join(order, ","); got != "second,first" { + t.Errorf("releases ran %q, want %q (reverse acquisition order)", got, "second,first") + } + + order = nil + unlockAfterOne() + if got := strings.Join(order, ","); got != "first" { + t.Errorf("the earlier snapshot ran %q, want %q", got, "first") + } +} From c04edd661c0c7d9dd626b3602c4a90fbd5cbe8c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:00:57 +0000 Subject: [PATCH 5/8] fix: gitignore the per-machine dependency overlay in created repos deps/Brewfile..local is documented as belonging to one machine only, but the generated .gitignore covered only ferry.local.toml and local/, so sync committed the overlay, every clone installed it via apply --deps, and bundle export carried it. The ignore pattern now ships from the single chokepoint every init route uses, and the init --github pre-create gate model is rendered from the writer's own pattern set so the two can never diverge. Assisted-by: Claude:claude-fable-5 --- cmd/init.go | 31 ++++- cmd/init_github.go | 4 +- cmd/init_gitignore_test.go | 143 ++++++++++++++++++++++++ evals/deps_local_overlay_ignore_test.go | 134 ++++++++++++++++++++++ 4 files changed, 307 insertions(+), 5 deletions(-) create mode 100644 cmd/init_gitignore_test.go create mode 100644 evals/deps_local_overlay_ignore_test.go diff --git a/cmd/init.go b/cmd/init.go index 69ef00a..b0d17e9 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -891,10 +891,33 @@ func existingConfiguredRepo() (string, bool) { return mc.Repo, true } +// localLayerIgnorePatterns is the FULL set of .gitignore entries every repo ferry +// creates or adopts must carry — the per-machine layer that belongs to ONE machine +// and must never be committed: +// +// - ferry.local.toml (config.LocalManifestName) — per-machine scope overrides +// - local/ — the per-machine file layer +// - deps/Brewfile.*.local — the per-machine deps overlay +// +// The deps overlay glob is root-anchored by its embedded slash, so it matches +// deps/Brewfile.darwin.local and deps/Brewfile.linux.local (and only those) at the +// repo root. Without it `ferry sync`'s `git add -A` would commit a machine's +// private overlay, every other machine would clone and install it via +// `apply --deps`, and `bundle export` (tracked-set driven) would ship it — the +// opposite of the documented per-machine promise. +var localLayerIgnorePatterns = []string{config.LocalManifestName, "local/", "deps/Brewfile.*.local"} + +// plannedGitignoreBody is the .gitignore body a FRESH repo receives — the same +// bytes ensureLocalLayerIgnored appends to an empty/absent file. init --github's +// pre-create secret gate models the initial commit with it, so the gate scans what +// is actually written (lockstep with the writer). +var plannedGitignoreBody = strings.Join(localLayerIgnorePatterns, "\n") + "\n" + // ensureLocalLayerIgnored makes sure the repo's .gitignore excludes the per-machine -// .local layer (ferry.local.toml and local/). It is idempotent: existing entries -// are kept and only the missing ones are appended, so it never disturbs a repo that -// already ignores them. +// layer (localLayerIgnorePatterns: ferry.local.toml, local/ and the +// deps/Brewfile.*.local overlay). It is idempotent: existing entries are kept and +// only the missing ones are appended, so it never disturbs a repo that already +// ignores them. func ensureLocalLayerIgnored(repo string) error { gitignore := filepath.Join(repo, ".gitignore") // Guard the FULL .gitignore path BEFORE any read or write: a repo @@ -915,7 +938,7 @@ func ensureLocalLayerIgnored(repo string) error { } var add []string - for _, want := range []string{config.LocalManifestName, "local/"} { + for _, want := range localLayerIgnorePatterns { if !have[want] { add = append(add, want) } diff --git a/cmd/init_github.go b/cmd/init_github.go index 0e67065..c679a2e 100644 --- a/cmd/init_github.go +++ b/cmd/init_github.go @@ -352,7 +352,9 @@ func gateManagedContentBeforeCommit(plan *seedPlan) error { func plannedCommitContents(plan *seedPlan) map[string]string { files := map[string]string{ config.SharedManifestName: plan.manifest, - ".gitignore": config.LocalManifestName + "\nlocal/\n", + // Rendered from the SAME pattern set ensureLocalLayerIgnored writes, so the + // gate's model of the initial commit can never drift from the real writer. + ".gitignore": plannedGitignoreBody, } if plan.shared != nil { files[dotfile.RepoSubdir+"/zshrc"] = string(plan.shared) diff --git a/cmd/init_gitignore_test.go b/cmd/init_gitignore_test.go new file mode 100644 index 0000000..b71a1de --- /dev/null +++ b/cmd/init_gitignore_test.go @@ -0,0 +1,143 @@ +package cmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/REPPL/ferry/internal/config" +) + +// ignoredLines returns the trimmed, non-empty lines of the repo's .gitignore. +func ignoredLines(t *testing.T, repo string) []string { + t.Helper() + data, err := os.ReadFile(filepath.Join(repo, ".gitignore")) + if err != nil { + t.Fatalf("read .gitignore: %v", err) + } + var out []string + for _, line := range strings.Split(string(data), "\n") { + if s := strings.TrimSpace(line); s != "" { + out = append(out, s) + } + } + return out +} + +// wantIgnorePatterns is the FULL per-machine ignore set every repo ferry creates +// or adopts must carry: the local manifest, the local/ layer, and the per-machine +// deps overlay deps/Brewfile..local. The overlay is documented as a +// per-machine, gitignored file ("belongs to one machine only"); if it is not +// ignored, `ferry sync`'s `git add -A` commits it, every other machine clones and +// installs it via `apply --deps`, and `bundle export` (tracked-set driven) ships +// it — the exact opposite of the documented promise. +var wantIgnorePatterns = []string{config.LocalManifestName, "local/", "deps/Brewfile.*.local"} + +// TestEnsureLocalLayerIgnored_CoversPerMachineDepsOverlay is the regression for +// the missing deps overlay pattern: the .gitignore ferry writes must ignore all +// three per-machine artefacts. +func TestEnsureLocalLayerIgnored_CoversPerMachineDepsOverlay(t *testing.T) { + repo := t.TempDir() + if err := ensureLocalLayerIgnored(repo); err != nil { + t.Fatalf("ensureLocalLayerIgnored: %v", err) + } + got := ignoredLines(t, repo) + for _, want := range wantIgnorePatterns { + if !containsLine(got, want) { + t.Errorf("fresh .gitignore does not ignore %q (per-machine artefact would be committed); got %v", want, got) + } + } +} + +// TestEnsureLocalLayerIgnored_IdempotentAndPreserving pins the append-only, +// idempotent contract: a second call adds nothing, and pre-existing user entries +// (including one of the wanted patterns already present) survive untouched. +func TestEnsureLocalLayerIgnored_IdempotentAndPreserving(t *testing.T) { + repo := t.TempDir() + // A repo that already ignores something of its own AND already carries one of + // ferry's patterns (no trailing newline, to exercise the newline fixup). + seed := "# user entries\nnotes.private\ndeps/Brewfile.*.local" + if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte(seed), 0o644); err != nil { + t.Fatal(err) + } + if err := ensureLocalLayerIgnored(repo); err != nil { + t.Fatalf("ensureLocalLayerIgnored: %v", err) + } + first := ignoredLines(t, repo) + if !containsLine(first, "notes.private") { + t.Errorf("pre-existing user entry was dropped: %v", first) + } + for _, want := range wantIgnorePatterns { + if !containsLine(first, want) { + t.Errorf(".gitignore does not ignore %q after adoption; got %v", want, first) + } + } + if n := countLine(first, "deps/Brewfile.*.local"); n != 1 { + t.Errorf("already-present pattern duplicated %d times: %v", n, first) + } + + before, err := os.ReadFile(filepath.Join(repo, ".gitignore")) + if err != nil { + t.Fatal(err) + } + if err := ensureLocalLayerIgnored(repo); err != nil { + t.Fatalf("ensureLocalLayerIgnored (second call): %v", err) + } + after, err := os.ReadFile(filepath.Join(repo, ".gitignore")) + if err != nil { + t.Fatal(err) + } + if string(before) != string(after) { + t.Errorf("ensureLocalLayerIgnored is not idempotent:\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +// TestPlannedCommitGate_GitignoreMatchesWriter keeps the pre-create secret gate's +// MODEL of the initial commit in lockstep with what ensureLocalLayerIgnored +// actually writes: the planned .gitignore must carry the same pattern set, or the +// gate scans a file that differs from the one committed. +func TestPlannedCommitGate_GitignoreMatchesWriter(t *testing.T) { + planned := plannedCommitContents(declareOnlyPlan(""))[".gitignore"] + for _, want := range wantIgnorePatterns { + if !containsLine(ignoreLinesOf(planned), want) { + t.Errorf("plannedCommitContents .gitignore missing %q (gate model diverges from ensureLocalLayerIgnored); got %q", want, planned) + } + } + // Byte-for-byte agreement with the real writer on a fresh repo. + repo := t.TempDir() + if err := ensureLocalLayerIgnored(repo); err != nil { + t.Fatalf("ensureLocalLayerIgnored: %v", err) + } + written, err := os.ReadFile(filepath.Join(repo, ".gitignore")) + if err != nil { + t.Fatal(err) + } + if string(written) != planned { + t.Errorf("planned .gitignore != written .gitignore\nplanned: %q\nwritten: %q", planned, written) + } +} + +func ignoreLinesOf(body string) []string { + var out []string + for _, line := range strings.Split(body, "\n") { + if s := strings.TrimSpace(line); s != "" { + out = append(out, s) + } + } + return out +} + +func containsLine(lines []string, want string) bool { + return countLine(lines, want) > 0 +} + +func countLine(lines []string, want string) int { + n := 0 + for _, l := range lines { + if l == want { + n++ + } + } + return n +} diff --git a/evals/deps_local_overlay_ignore_test.go b/evals/deps_local_overlay_ignore_test.go new file mode 100644 index 0000000..2c311d6 --- /dev/null +++ b/evals/deps_local_overlay_ignore_test.go @@ -0,0 +1,134 @@ +package evals + +// Behavioural eval for the per-machine deps overlay promise: deps/Brewfile..local +// "belongs to one machine only" — it must never be committed or published. The repos +// ferry CREATES must therefore ignore it out of the box, or `ferry sync`'s `git add -A` +// commits a machine's private overlay, every other machine clones and installs it via +// `apply --deps`, and `bundle export` (tracked-set driven) ships it. +// +// Drives the REAL binary (skips when FERRY_BIN is unset) against a LOCAL BARE GIT +// REPO as origin — no network, no real GitHub. + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// TestFreshInitIgnoresPerMachineDepsOverlay: a fresh `ferry init` repo must leave +// a written deps/Brewfile..local UNTRACKED across a real `ferry sync` — the +// sync commit must not contain it and it must not reach the bare origin. +func TestFreshInitIgnoresPerMachineDepsOverlay(t *testing.T) { + t.Parallel() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH: init/sync evals need git") + } + s := NewSandbox(t) + + // Fresh init at an explicit destination: ferry writes the repo AND its .gitignore. + if _, errOut, code := s.FerryWithInput("", "init", "--fresh", s.Repo); code != 0 { + t.Fatalf("`ferry init --fresh` exited %d\n%s", code, errOut) + } + + // The per-machine overlay this machine would use (deps/Brewfile..local). + overlayRel := "deps/Brewfile." + runtime.GOOS + ".local" + const overlayMarker = "PER_MACHINE_ONLY_OVERLAY_MARKER" + + // (1) Cheap, direct check: git itself must consider the path ignored in the + // repo ferry just created. + if err := os.MkdirAll(filepath.Join(s.Repo, "deps"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(s.Repo, overlayRel), + []byte("# "+overlayMarker+"\nbrew \"ripgrep\"\n"), 0o644); err != nil { + t.Fatal(err) + } + if out, err := runGitIn(s.Repo, "check-ignore", "-q", "--", overlayRel); err != nil { + ign, _ := os.ReadFile(filepath.Join(s.Repo, ".gitignore")) + t.Errorf("fresh init repo does NOT ignore %s (git check-ignore: %v %s)\n.gitignore:\n%s", + overlayRel, err, strings.TrimSpace(out), ign) + } + + // (2) End-to-end: a real `ferry sync` must not commit or publish it. + bare := t.TempDir() + syncGit(t, bare, "init", "-q", "--bare", "-b", syncBranch, ".") + syncGit(t, s.Repo, "config", "user.email", "eval@localhost") + syncGit(t, s.Repo, "config", "user.name", "eval") + syncGit(t, s.Repo, "add", "-A") + syncGit(t, s.Repo, "commit", "-q", "--allow-empty", "-m", "baseline") + syncGit(t, s.Repo, "remote", "add", "origin", bare) + syncGit(t, s.Repo, "push", "-q", "origin", syncBranch) + syncGit(t, s.Repo, "fetch", "-q", "origin") + + // A tracked change so sync has real work to commit and push alongside the overlay. + s.WriteRepoFile(t, "shared.txt", "line-1\n") + + if _, errOut, code := s.FerryEnvWithInput("y\n", []string{allowFileOrigin}, + "sync", "--allow-unmanaged"); code != 0 { + t.Fatalf("`ferry sync` exited %d\n%s", code, errOut) + } + + // The overlay must still be untracked in the working clone... + // (`ls-files -- ` lists the path only when it is in the index.) + if tracked, ok := syncGitOK(t, s.Repo, "ls-files", "--", overlayRel); ok && strings.TrimSpace(tracked) != "" { + t.Errorf("`ferry sync` TRACKED the per-machine overlay %s (it must stay untracked): %s", overlayRel, tracked) + } + // ...and must not appear anywhere in the pushed history on the origin. + if out, ok := syncGitOK(t, bare, "log", "--all", "--name-only", "--pretty=format:"); ok { + for _, line := range strings.Split(out, "\n") { + if strings.TrimSpace(line) == overlayRel { + t.Errorf("the per-machine overlay %s was PUBLISHED to the origin (it belongs to one machine only)", overlayRel) + } + } + } + // Belt and braces: no object in the origin's store carries the overlay's marker. + assertMarkerNotInBareObjects(t, bare, overlayMarker) + + // The overlay's content on this machine survives untouched. + body, err := os.ReadFile(filepath.Join(s.Repo, overlayRel)) + if err != nil || !strings.Contains(string(body), overlayMarker) { + t.Errorf("the per-machine overlay was lost or rewritten locally: %v %q", err, body) + } +} + +// assertMarkerNotInBareObjects scans EVERY object in a BARE repo's store +// (reachable + unreachable/dangling) for the needle — proving "no OBJECTS carry +// this content", not merely "no ref points at it". A no-op when git is absent. +func assertMarkerNotInBareObjects(t *testing.T, bare, needle string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + return + } + env := gitIsolatedEnv("GIT_PAGER=cat") + ids := map[string]bool{} + collect := func(args ...string) { + c := exec.Command("git", append([]string{"-C", bare}, args...)...) + c.Env = env + out, _ := c.CombinedOutput() + for _, tok := range strings.Fields(string(out)) { + if isHexish(tok) { + ids[tok] = true + } + } + } + collect("rev-list", "--all", "--objects", "--reflog") + fsck := exec.Command("git", "-C", bare, "fsck", "--unreachable", "--dangling") + fsck.Env = env + fout, _ := fsck.CombinedOutput() + for _, line := range strings.Split(string(fout), "\n") { + if f := strings.Fields(line); len(f) >= 3 && isHexish(f[2]) { + ids[f[2]] = true + } + } + for id := range ids { + cat := exec.Command("git", "-C", bare, "cat-file", "-p", id) + cat.Env = env + if body, err := cat.CombinedOutput(); err == nil && strings.Contains(string(body), needle) { + t.Errorf("the per-machine overlay's content reached the origin: needle found in bare object %s", id) + return + } + } +} From f7ca352d3f8145798c37fa64b5735b06717ebc25 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:01:09 +0000 Subject: [PATCH 6/8] fix: recoverable release publish, complete-release healing, CI hardening A failed post-publish check left the release run permanently red (the create errors once the Release exists) while a full auto-release re-run saw the release as done and skipped the bypassed gates; publishing is now idempotent and the heal predicate requires a published, asset- complete release. The zizmor SARIF upload is skipped on fork pull requests where the token is read-only (the audit still gates), the consistency lint's old-path check exempts only Go test files, install.sh names a failed binary download, the checksums Makefile comment states that the build prerequisite rebuilds with this invocation's VERSION, and the ci.yml cross-compile comment stops claiming a pre-tag gate that auto-release's concurrent tag outruns. Assisted-by: Claude:claude-fable-5 --- .github/workflows/auto-release.yml | 55 +++++++++++++++++++----------- .github/workflows/ci.yml | 26 ++++++++++---- .github/workflows/release.yml | 30 ++++++++++++++-- Makefile | 10 ++++-- install.sh | 8 ++++- scripts/consistency-lint.sh | 9 ++--- 6 files changed, 103 insertions(+), 35 deletions(-) diff --git a/.github/workflows/auto-release.yml b/.github/workflows/auto-release.yml index 62f5232..40cb29f 100644 --- a/.github/workflows/auto-release.yml +++ b/.github/workflows/auto-release.yml @@ -11,12 +11,13 @@ name: auto-release # Only the NEWEST dated version is ever tagged: older CHANGELOG versions are left # alone, because tagging them at the current HEAD would mis-point an immutable # tag at the wrong code. Idempotent: when the newest version is already tagged AND -# its GitHub Release exists, `detect` reports need_tag=need_release=false and -# nothing runs, so an ordinary (non-release) push to main does nothing. If the tag -# exists but its Release is MISSING (e.g. a transient publish failure), `detect` -# sets need_release=true and re-invokes `release` ALONE — built from the tagged -# commit (release_ref), never the moved-on HEAD — so a flaky publish never -# permanently wedges the version. +# its GitHub Release is published and complete, `detect` reports +# need_tag=need_release=false and nothing runs, so an ordinary (non-release) push +# to main does nothing. If the tag exists but its Release is MISSING OR +# INCOMPLETE — absent, still a draft, or short of its five assets, as a transient +# publish failure leaves it — `detect` sets need_release=true and re-invokes +# `release` ALONE — built from the tagged commit (release_ref), never the +# moved-on HEAD — so a flaky publish never permanently wedges the version. on: push: branches: [main] @@ -81,21 +82,37 @@ jobs: tag="v$version" if git rev-parse -q --verify "refs/tags/$tag" >/dev/null; then # The tag already exists, so it is NEVER moved (immutable). Re-release - # ONLY when its GitHub Release is missing: a transient publish failure - # must not permanently wedge the version. Build the re-release FROM the + # ONLY when its GitHub Release is missing or incomplete: a transient + # publish failure must not permanently wedge the version, and a + # half-published one must not pass for done. Build the re-release FROM the # tagged commit, not the current (moved-on) main HEAD — resolve the tag # to its immutable commit SHA and hand it to release.yml as `ref`. echo "need_tag=false" >> "$GITHUB_OUTPUT" + # "Released" means released AND complete, not merely present. A run + # that dies part-way through publishing leaves a Release that EXISTS + # — draft, or missing some of its assets — and a bare existence check + # calls that done, so the version stays half-published while every + # post-publish gate in release.yml is silently skipped. Require + # isDraft=false and the full asset set: the four cross-compiled + # binaries plus checksums.txt. The comparison is >= 5, not == 5, so + # an asset attached by hand cannot make a complete release look + # incomplete and re-release it on every subsequent push to main. + # # Deliberate fail-open: ANY non-zero from `gh release view` (a true 404 - # or a transient rate-limit/auth blip) counts as "Release missing" and - # re-releases. Safe — `gh release create` (release.yml) has no --clobber, - # so a false positive just errors on the existing release and the - # success-gated prune never runs: one red run, no data loss. Parsing the - # error to isolate a real 404 was rejected — it would hinge on gh's - # wording and could misclassify a genuine missing-Release, re-wedging the - # very failure this heals. - if gh release view "$tag" >/dev/null 2>&1; then - echo "$tag is tagged and released; nothing to do." + # or a transient rate-limit/auth blip), and any output that is not a + # literal "true", counts as "Release missing or incomplete" and + # re-releases. Safe — release.yml's publish step is idempotent: it + # re-uploads the assets with --clobber and re-asserts the title, + # notes, draft state and prerelease flag when the Release already + # exists, so a false positive republishes the same bytes and re-runs + # the post-publish gates instead of erroring. Parsing the error to + # isolate a real 404 was rejected — it would hinge on gh's wording and + # could misclassify a genuine missing-Release, re-wedging the very + # failure this heals. + released="$(gh release view "$tag" --json isDraft,assets \ + --jq '((.isDraft | not) and ((.assets | length) >= 5))' 2>/dev/null || true)" + if [ "$released" = "true" ]; then + echo "$tag is tagged and fully released; nothing to do." echo "need_release=false" >> "$GITHUB_OUTPUT" else commit="$(git rev-parse --verify "$tag^{commit}")" @@ -130,10 +147,10 @@ jobs: [ -n "$rl_fails" ] || rl_fails=0 attempts=$((ar_fails + rl_fails)) if [ "$attempts" -ge 3 ]; then - echo "::error::$tag is tagged but has no GitHub Release after $attempts failed release runs — NOT retrying again. The tagged commit fails the release gate deterministically; see docs/how-to/cutting-a-release.md (Manual / recovery flow)." + echo "::error::$tag is tagged but has no complete GitHub Release after $attempts failed release runs — NOT retrying again. The tagged commit fails the release gate deterministically; see docs/how-to/cutting-a-release.md (Manual / recovery flow)." echo "need_release=false" >> "$GITHUB_OUTPUT" else - echo "$tag is tagged but has NO GitHub Release ($attempts prior failed attempt(s)); re-releasing from $commit." + echo "$tag is tagged but has no complete GitHub Release ($attempts prior failed attempt(s)); re-releasing from $commit." echo "need_release=true" >> "$GITHUB_OUTPUT" echo "release_ref=$commit" >> "$GITHUB_OUTPUT" fi diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f8d6b7..72b1768 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -87,12 +87,17 @@ jobs: run: go build ./... # `go build ./...` compiles only the host target, but a release ships all - # four Makefile TARGETS — and the first `make build` on the release path - # runs only AFTER auto-release has pushed the (immutable) tag, so a - # target-specific break would make that version permanently unreleasable. - # Cross-compile every target here, pre-tag. Output is gitignored, so the - # gen-docs staleness check above is unaffected; one leg suffices because - # cross-compilation is host-independent. + # four Makefile TARGETS, so cross-compile every one of them here. This + # catches a target-specific break on PR and branch runs — before the + # commit that would promote a version reaches main. It does NOT gate the + # tag: auto-release runs on the same main push, concurrently with this + # job, and tags within seconds; nothing sequences the two. release.yml's + # own `verify` re-runs `make build` post-tag, so a break that slips + # through spends the version number — see the spent-version recovery in + # docs/how-to/cutting-a-release.md (Manual / recovery flow). + # Output is gitignored, so the gen-docs staleness check above is + # unaffected; one leg suffices because cross-compilation is + # host-independent. - name: Cross-compile every release target (make build) if: matrix.os == 'ubuntu-latest' run: make build @@ -236,4 +241,11 @@ jobs: # ferry is public, so zizmor uploads its SARIF to Code Scanning, surfacing # findings in the Security tab; the job also still gates on those findings. # The upload needs security-events: write from the job permissions above. - advanced-security: true + # A pull_request run from a FORK carries a read-only token, so the upload + # would 403 and redden a job the contributor has no way to fix. Disable + # the upload — and only the upload — on fork PRs: zizmor still runs and + # still fails the job on its findings, so the gate is unchanged; the + # findings simply appear in the job log rather than the Security tab. + # The expression is true for every non-PR event (push, schedule) and for + # a PR whose head repo IS this repo (a branch PR from a collaborator). + advanced-security: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 70abd5a..c8d9326 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -415,14 +415,40 @@ jobs: # contain a hyphen (`v1.2.3+2026-01-01`) and is not a prerelease. A plain # vX.Y.Z tag leaves the array empty, so the command is byte-identical to # a normal release's. + # + # IDEMPOTENT by branch, because three gates run AFTER this step + # (attestation verify, fresh-download checksum verify, prune) and any of + # them can go red on a transient blip. Recovery is "re-run failed jobs", + # which replays this step against a release that already exists — a bare + # `gh release create` answers that with a 422 and the run stays wedged + # red forever. So: if the Release is already there, re-upload the assets + # (--clobber, since the rebuilt binaries are byte-identical only if the + # build is reproducible) and re-assert its title, notes, draft state and + # prerelease flag, leaving the post-publish gates free to run again. The + # create arm keeps --verify-tag; the upload arm cannot use it, but that + # arm only ever touches a Release GitHub already bound to this tag. run: | set -euo pipefail pre=() case "${TAG%%+*}" in *-*) pre=(--prerelease) ;; esac - gh release create "$TAG" bin/ferry-* bin/checksums.txt \ - --verify-tag --title "$TAG" --notes-file "$NOTES_FILE" "${pre[@]}" + if gh release view "$TAG" >/dev/null 2>&1; then + echo "release $TAG already exists — re-uploading assets and re-asserting its metadata (re-run of a failed post-publish step)." + gh release upload "$TAG" bin/ferry-* bin/checksums.txt --clobber + # `gh release edit` spells the prerelease flag as --prerelease=BOOL, + # so translate the create arm's presence/absence array into an + # explicit true/false — an existing release must never silently keep + # a stale flag. + prerelease=false + [ "${#pre[@]}" -eq 0 ] || prerelease=true + gh release edit "$TAG" \ + --draft=false --title "$TAG" --notes-file "$NOTES_FILE" \ + --prerelease="$prerelease" + else + gh release create "$TAG" bin/ferry-* bin/checksums.txt \ + --verify-tag --title "$TAG" --notes-file "$NOTES_FILE" "${pre[@]}" + fi env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/Makefile b/Makefile index 3b1acd7..8b66830 100644 --- a/Makefile +++ b/Makefile @@ -55,8 +55,14 @@ clean: rm -rf $(BINDIR) # Write bin/checksums.txt: the SHA256 of every binary in sha256sum format. -# Depends on build so the binaries exist. The release workflow uploads this file -# as a release asset; install.sh fetches it from the release and verifies each +# `build` is .PHONY, so the prerequisite does not merely ensure the binaries +# exist — it REBUILDS all four with THIS invocation's VERSION, overwriting +# whatever an earlier make left in bin/. An empty VERSION (the default) restamps +# them with the in-source dev version, so hashing them yields a manifest that is +# internally consistent but does NOT match the release: pass the version to this +# same invocation (`make checksums VERSION=vX.Y.Z`) whenever the hashes are meant +# to match a published release. The release workflow uploads this file as a +# release asset; install.sh fetches it from the release and verifies each # download against it. checksums: build @scripts/gen-checksums.sh diff --git a/install.sh b/install.sh index 62d25fd..5241975 100644 --- a/install.sh +++ b/install.sh @@ -131,7 +131,13 @@ else exit 1 fi echo "ferry install: downloading ${asset} (${VERSION})..." - curl -fsSL "${base_url}/${asset}" -o "$tmp" + # Same fail-loud shape as the checksums fetch above: -s suppresses curl's own + # message, so a 404 (no asset for this target in this release) would otherwise + # kill the script under set -e with nothing on stderr to explain it. + if ! curl -fsSL "${base_url}/${asset}" -o "$tmp"; then + echo "ferry install: could not download ${asset} from the release; refusing to install" >&2 + exit 1 + fi from_network=1 fi diff --git a/scripts/consistency-lint.sh b/scripts/consistency-lint.sh index bf829a3..c40be61 100755 --- a/scripts/consistency-lint.sh +++ b/scripts/consistency-lint.sh @@ -8,9 +8,10 @@ # 2. Every ADR is named sequentially as NNNN-title.md, never --. # 3. No prose or config file points session handoff at .abcd/work/NEXT.md — the # handoff lives in the private .abcd/.work.local/ layer (ADR 0002). Scope is the -# human/agent-facing surface (docs, scripts, hooks, templates); Go sources -# are exempt because their tests legitimately assert the OLD path is now -# absent. The developer-record .abcd/development/plans/ and research/ (which +# human/agent-facing surface (docs, scripts, hooks, templates) and Go +# sources; only Go TEST files are exempt, because they legitimately assert +# the OLD path is now absent (the same test-file carve-out invariant 4 +# makes). The developer-record .abcd/development/plans/ and research/ (which # describe past and investigative state) and the changelog are exempt # too (they describe past state), as is this script itself. # 4. No Go source outside internal/dotfile/dotfile.go re-inlines either @@ -53,7 +54,7 @@ if [ -d "$decisions" ]; then fi # 3. stale .abcd/work/NEXT.md handoff references (must be .abcd/.work.local/NEXT.md) -hits=$(git ls-files -- ':!.abcd/development/plans/' ':!.abcd/development/research/' ':!CHANGELOG.md' ':!scripts/consistency-lint.sh' ':!*.go' \ +hits=$(git ls-files -- ':!.abcd/development/plans/' ':!.abcd/development/research/' ':!CHANGELOG.md' ':!scripts/consistency-lint.sh' ':!*_test.go' \ | xargs -r grep -nF '.abcd/work/NEXT.md' 2>/dev/null || true) if [ -n "$hits" ]; then echo "consistency-lint: session handoff is .abcd/.work.local/NEXT.md, not .abcd/work/NEXT.md:" >&2 From 141432f5521953f82acfe04f84ea3db17100e2dd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:01:09 +0000 Subject: [PATCH 7/8] docs: correct reference, tutorial, and help text against the code The compatibility contract names restore snapshots as the second version-independent store; the configuration reference states the iTerm2 allowlist is compiled in, lists all eight kept categories, gives the committed-plist route for extra keys, and documents the gitignored deps overlay with its untracking step; the single-branch main constraint on sync enters the commands reference, the tutorial, and the sync help; route jargon is replaced with plain language; ssh.md states the ~/.ssh invariant universally including the cargo- store guard; the release how-to corrects the failed-check claim, documents post-publish recovery and the local NEXT.md reset, and stamps the by-hand checksum recipe; the scaffold help names exactly what it creates; AGENTS.md's CI list names the gitleaks and zizmor gates; the tutorial distinguishes local drift from a conflict. Assisted-by: Claude:claude-fable-5 --- AGENTS.md | 4 ++- cmd/agents.go | 6 ++-- cmd/commands.go | 16 ++++++----- docs/explanation/ssh.md | 11 ++++---- docs/how-to/cutting-a-release.md | 31 +++++++++++++++++---- docs/reference/cli/ferry_agents_scaffold.md | 6 ++-- docs/reference/cli/ferry_sync.md | 16 ++++++----- docs/reference/commands.md | 4 +-- docs/reference/compatibility.md | 23 ++++++++++----- docs/reference/configuration.md | 30 +++++++++++++++----- docs/tutorials/getting-started.md | 20 +++++++++---- 11 files changed, 115 insertions(+), 52 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e92730d..c926280 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,9 @@ Without `FERRY_BIN` the eval suite skips every behavioural test and passes. CI (`.github/workflows/ci.yml`) runs gofmt, build, the cross-compile of every release target, vet, `go test ./...`, race tests on the internal packages, the CLI-reference staleness check, the consistency lint, and the full eval suite -against real Linux and macOS binaries. +against real Linux and macOS binaries, plus a full-history secret scan +(gitleaks) and an audit of the workflows themselves (zizmor) — both blocking, +with no local equivalent. `make gen-docs` regenerates the committed CLI reference under `docs/reference/cli/` — run it after any command, flag, or help-text change, diff --git a/cmd/agents.go b/cmd/agents.go index 1262035..e1730ba 100644 --- a/cmd/agents.go +++ b/cmd/agents.go @@ -43,8 +43,10 @@ never touched). Default mode (your own repo — tracked files): an AGENTS.md router stamped from the config repo's template ({{PROJECT}}/{{DATE}} substituted), CLAUDE.md and GEMINI.md as relative symlinks to AGENTS.md inside the repo, a committed -.abcd/work/ (DECISIONS.md, CONTEXT.md), the docs/ hierarchy with its map -(docs/README.md), and a pre-commit config when the repo has none. +.abcd/work/ (DECISIONS.md, CONTEXT.md), the user-facing docs map (docs/README.md; +the Diátaxis content dirs are created on first use), the .abcd/development/ +record dirs (plans, research, decisions), and a pre-commit config when the repo +has none. --attribution (tracked mode only) marks a repo that REQUIRES AI disclosure, overriding the workspace no-attribution default: it installs the diff --git a/cmd/commands.go b/cmd/commands.go index a5694da..5d900f6 100644 --- a/cmd/commands.go +++ b/cmd/commands.go @@ -59,13 +59,15 @@ var syncCmd = &cobra.Command{ Short: "Publish captured changes and pull remote ones for a managed repo", Long: `Publish local changes and pull remote ones in one command. -sync is the everyday update for a managed (route-2) repo: it pulls remote work, -commits your locally-captured changes, and pushes them — WITHOUT ever losing -local work or force-pushing. It integrates the remote first from a clean -baseline, gates the whole push range for secrets, and pushes a single explicit -ref. On a conflict it leaves your machine byte-for-byte unchanged and asks you -to resolve with git. It never runs apply — run "ferry apply" afterwards to -deploy the pulled changes.`, +sync is the everyday update for a managed repo (one set up by +"ferry init --github"): it pulls remote work, commits your locally-captured +changes, and pushes them — WITHOUT ever losing local work or force-pushing. It +integrates the remote first from a clean baseline, gates the whole push range +for secrets, and pushes a single explicit ref. ferry manages a single-branch +config repo, so sync integrates and pushes "main" and refuses to run on a +detached HEAD or with any other branch checked out. On a conflict it leaves +your machine byte-for-byte unchanged and asks you to resolve with git. It never +runs apply — run "ferry apply" afterwards to deploy the pulled changes.`, RunE: runSync, } diff --git a/docs/explanation/ssh.md b/docs/explanation/ssh.md index 5dc227c..586b606 100644 --- a/docs/explanation/ssh.md +++ b/docs/explanation/ssh.md @@ -24,11 +24,12 @@ Staying hands-off removes this entire class of risk rather than trying to mitiga ## The invariant -`~/.ssh/` is untouchable across every ferry operation: `apply`, `capture`, `status`, -`diff`, and `restore` all skip it. A write whose target resolves under `~/.ssh` — even -through a symlinked parent directory — is refused by the containment check, so no ferry -path can ever land a file there. `ferry doctor` can *report* (read-only) when key or -directory permissions look wrong, but it never changes them. +`~/.ssh/` is untouchable across every ferry operation: no command reads, writes, or +captures anything under it. A write whose target resolves under `~/.ssh` — even +through a symlinked parent directory — is refused by the containment check, so no +ferry path can ever land a file there, and the `work` verbs refuse a cargo store that +is, or resolves into, `~/.ssh` before touching it. `ferry doctor` can *report* +(read-only) when key or directory permissions look wrong, but it never changes them. ## What about syncing config across machines? diff --git a/docs/how-to/cutting-a-release.md b/docs/how-to/cutting-a-release.md index 8a7079e..41bb9e6 100644 --- a/docs/how-to/cutting-a-release.md +++ b/docs/how-to/cutting-a-release.md @@ -66,9 +66,12 @@ left un-shipped fails the release even on a hand-pushed tag): CHANGELOG section, extracted verbatim, plus a link to the full CHANGELOG at that tag; a version whose section cannot be found publishes with an empty body rather than blocking the release. -5. Proves the attestation by downloading a binary fresh from the new Release and running - `gh attestation verify` against it. A failed attestation or verification fails the - release. +5. Proves the published assets by downloading them fresh from the new Release: `gh + attestation verify` against a binary, then `sha256sum -c checksums.txt` across the + lot — the same pairing `install.sh` relies on. These checks run **after** the + Release is public, so a failure fails the workflow *run*, not the release: the + assets stay published and the red run is the signal to inspect them. See + [post-publish recovery](#manual--recovery-flow) below. The workflow pushes nothing to any branch. It records the remote default-branch tip at the start of the release job and asserts it is unchanged at the end, so a step that ever @@ -121,6 +124,20 @@ Land the fix on `main`, promote the **next** patch version in the CHANGELOG, and the automatic flow ship that instead. Tags are never deleted (see the pruning rules above), so the failed tag simply remains, release-less. +**A run that goes red after the Release is published.** The three post-publish steps — +the attestation verification, the fresh-download `sha256sum -c`, and the prune — run +once the Release is already public, so a failure there reddens the run without +unpublishing anything. Re-run the failed jobs: the publish step is idempotent (given an +existing Release it re-uploads the assets and re-asserts the title, notes, draft state +and prerelease flag), so the post-publish steps get another attempt against the same +release. The by-hand equivalents are the checksum check, from a directory holding the +assets downloaded fresh from the Release, and the prune, from the repository: + +```bash +sha256sum -c checksums.txt # or: shasum -a 256 -c checksums.txt +scripts/prune-releases.sh --current vX.Y.Z # add --dry-run to preview +``` + When `auto-release` is disabled, or a tag must be cut by hand, [`scripts/release.sh`](../../scripts/release.sh) is the blessed driver, run from a clean `main` that is up to date with `origin/main`: @@ -158,11 +175,13 @@ publishes; omitting it is fine only for a mechanics check of the manifest. It th points back at the two publishing paths — the promotion push for the automatic flow, or `scripts/release.sh` as the recovery driver. -You can also run the pieces directly: +Or run the manifest step on its own. `checksums` rebuilds the four binaries first, so +it takes the same `VERSION` in the same invocation: a stamped build from an earlier, +separate `make build VERSION=vX.Y.Z` does not survive, because the rebuild replaces +those binaries with dev-stamped ones before hashing them. ```bash -make build VERSION=vX.Y.Z # cross-compile the four binaries -make checksums # write bin/checksums.txt over them +make checksums VERSION=vX.Y.Z # cross-compile the four binaries, then hash them ``` To publish by hand, verify a download against the manifest the same way `install.sh` diff --git a/docs/reference/cli/ferry_agents_scaffold.md b/docs/reference/cli/ferry_agents_scaffold.md index 033752a..a353358 100644 --- a/docs/reference/cli/ferry_agents_scaffold.md +++ b/docs/reference/cli/ferry_agents_scaffold.md @@ -13,8 +13,10 @@ never touched). Default mode (your own repo — tracked files): an AGENTS.md router stamped from the config repo's template ({{PROJECT}}/{{DATE}} substituted), CLAUDE.md and GEMINI.md as relative symlinks to AGENTS.md inside the repo, a committed -.abcd/work/ (DECISIONS.md, CONTEXT.md), the docs/ hierarchy with its map -(docs/README.md), and a pre-commit config when the repo has none. +.abcd/work/ (DECISIONS.md, CONTEXT.md), the user-facing docs map (docs/README.md; +the Diátaxis content dirs are created on first use), the .abcd/development/ +record dirs (plans, research, decisions), and a pre-commit config when the repo +has none. --attribution (tracked mode only) marks a repo that REQUIRES AI disclosure, overriding the workspace no-attribution default: it installs the diff --git a/docs/reference/cli/ferry_sync.md b/docs/reference/cli/ferry_sync.md index 9783c29..a99778d 100644 --- a/docs/reference/cli/ferry_sync.md +++ b/docs/reference/cli/ferry_sync.md @@ -6,13 +6,15 @@ Publish captured changes and pull remote ones for a managed repo Publish local changes and pull remote ones in one command. -sync is the everyday update for a managed (route-2) repo: it pulls remote work, -commits your locally-captured changes, and pushes them — WITHOUT ever losing -local work or force-pushing. It integrates the remote first from a clean -baseline, gates the whole push range for secrets, and pushes a single explicit -ref. On a conflict it leaves your machine byte-for-byte unchanged and asks you -to resolve with git. It never runs apply — run "ferry apply" afterwards to -deploy the pulled changes. +sync is the everyday update for a managed repo (one set up by +"ferry init --github"): it pulls remote work, commits your locally-captured +changes, and pushes them — WITHOUT ever losing local work or force-pushing. It +integrates the remote first from a clean baseline, gates the whole push range +for secrets, and pushes a single explicit ref. ferry manages a single-branch +config repo, so sync integrates and pushes "main" and refuses to run on a +detached HEAD or with any other branch checked out. On a conflict it leaves +your machine byte-for-byte unchanged and asks you to resolve with git. It never +runs apply — run "ferry apply" afterwards to deploy the pulled changes. ``` ferry sync [flags] diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 26d6884..2e596a8 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -9,11 +9,11 @@ Every command is run as `ferry ` (e.g. `ferry init`). | `init --wizard=off\|interactive\|answers:` | Choose the first-run wizard mode. `off` skips it (the non-interactive adopt-and-extract fallback); `interactive` forces the TUI (needs a tty pair); `answers:` drives every wizard decision from a TOML answers file (same gates, preview, backup, and confirm, no tty needed; schema in [Configuration](configuration.md#the-wizard-answers-file)). Default (unset): interactive on a real tty pair, else `off`. | | `init --repair` | Opt into the wizard's repair review: hardcoded `/Users/` paths to `$HOME`, duplicate `PATH` exports, dead `source` lines: each fix is accepted or declined individually. Needs a running wizard, so it conflicts with `--wizard=off` and (in interactive mode) a non-tty run — unless `--wizard=answers:` drives it, which satisfies the consent requirement. | | `init --github [name]` | Create a **new private** GitHub repo via the `gh` CLI's existing auth and manage it as ferry's HTTPS remote. Needs `gh` authenticated; ferry stores no token. Always private, never reuses an existing repo, and won't push a file that looks like a secret: the wizard (or the non-interactive fallback) extracts detected secrets to the local store first, so only placeholders are committed and pushed. Add `--yes` to assent to the create-confirm (needed non-interactively). | -| `apply` | Reconcile this machine to the repo (deploy dotfiles, terminal settings). On a run that has changes it walks the pending work **grouped by domain** (each managed domain — dotfiles, agents, terminals, keybindings, emacs, iTerm2 profiles — under its own name), staying **quiet when safe** and **stopping when risky**: a safe change (creating a file where none exists, or updating a target whose live content still matches what ferry last deployed) applies automatically, while a *risky* change — overwriting a file that differs from the last-deployed baseline, adopting a pre-existing file, or deploying a value from the secret store — halts for confirmation. In the walkthrough you confirm a domain wholesale, drill into it to see each change's full diff, apply or skip a change this run, or skip it *always* (remembered per machine in the gitignored `.local` layer). A clean, in-sync apply prints one line. Non-interactively — or with `--skip-wizard` — nothing risky is applied unattended: risky changes **fail closed** (listed, refused, non-zero exit) while the safe subset still applies. Idempotent; safe to re-run. Dependencies install behind `apply --deps`, and only for the dependency domains declared under `[manage]`: `brew = true` for the Homebrew/apt install, `npm-globals = true` for global npm packages. | +| `apply` | Reconcile this machine to the repo (deploy dotfiles, terminal settings). On a run that has changes it walks the pending work **grouped by domain** (each managed domain — dotfiles, agents, terminals, keybindings, emacs, iTerm2 profiles — under its own name), staying **quiet when safe** and **stopping when risky**: a safe change (creating a file where none exists, or updating a target whose live content still matches what ferry last deployed) applies automatically, while a *risky* change — overwriting a file that differs from the last-deployed baseline, adopting a pre-existing file, deploying a value from the secret store, or a **conflict** (a file edited both locally and in the repo) — halts for confirmation. A conflict is surfaced with its diff like the others, but confirming it does not overwrite the local edit: resolve it with `ferry capture` (keep the local edit) or `ferry apply --force` (take the repo's version). In the walkthrough you confirm a domain wholesale, drill into it to see each change's full diff, apply or skip a change this run, or skip it *always* (remembered per machine in the gitignored `.local` layer). A clean, in-sync apply prints one line. Non-interactively — or with `--skip-wizard` — nothing risky is applied unattended: risky changes **fail closed** (listed, refused, non-zero exit) while the safe subset still applies. Idempotent; safe to re-run. Dependencies install behind `apply --deps`, and only for the dependency domains declared under `[manage]`: `brew = true` for the Homebrew/apt install, `npm-globals = true` for global npm packages. | | `apply --skip-wizard` | Skip the guided walkthrough (for experts and scripts): safe changes still auto-apply, but risky changes are refused rather than prompted — they never happen unattended. | | `apply --force` | Treat every risky change as confirmed (an explicit override) and overwrite uncaptured local edits on a conflict; the downstream data-loss guards still apply and warn. | | `capture` | Pull local changes back into the repo. Interactive: approve each change, route it *shared* (synced everywhere) or *local* (this machine only). For sources that reference stored secrets, capture compares against the rendered content and splices your edits back around the placeholders, so stored values never re-enter the repo and a store-routed secret never blocks its own round-trip. It also captures edits to deployed agent files (routing to their shared source or a `local/agents/` overlay, refusing a true divergence with a diff) and offers to adopt new agent-shaped files it finds; see [The agents domain](../explanation/agents.md). | -| `sync` | Publish captured changes and pull remote ones for a managed repo, in one command. Integrates the remote first, never force-pushes, gates the whole push range for secrets, and leaves your machine unchanged on a conflict. Route-1 repos need `--allow-unmanaged`. Run `ferry apply` after to deploy pulled changes. | +| `sync` | Publish captured changes and pull remote ones for a managed repo, in one command. Integrates the remote first, never force-pushes, gates the whole push range for secrets, and leaves your machine unchanged on a conflict. ferry manages a **single-branch** config repo: `sync` integrates and pushes `main`, and refuses to run on a detached HEAD or with any other branch checked out. A repo not marked managed — anything not set up by `ferry init --github` — needs `--allow-unmanaged`. Run `ferry apply` after to deploy pulled changes. | | `status` | Report config drift (what changed on this machine). | | `doctor` | Report machine/tool health, and observe ferry's managed-target invariants read-only: no deployed target is a symlink (ferry deploys regular-file copies), none resolves under `~/.ssh`, and every target resolves inside `$HOME`. A genuine breach is reported `[fail]` and exits non-zero; a machine with nothing managed yet is advisory. | | `diff` | Preview what `apply` would change. | diff --git a/docs/reference/compatibility.md b/docs/reference/compatibility.md index 14c9a16..d374b02 100644 --- a/docs/reference/compatibility.md +++ b/docs/reference/compatibility.md @@ -12,7 +12,7 @@ ferry has three surfaces a user or an automation depends on: |---|---| | **CLI** | The commands, subcommands, flags, exit codes, and the machine-relevant wording of their output. | | **`ferry.toml` schema** | The manifest keys and value shapes documented in [configuration](configuration.md) and [the agents domain](../explanation/agents.md) — `[manage]`, `[agents]`, `[terminals]`, and their tables. | -| **On-disk state** | The files ferry keeps under its state directory (`~/.local/state/ferry`) — the last-applied store, the agents target record, the backup journal, the immutable baseline, and the work-domain state — plus the files `ferry work` writes into the configured cargo store, which cross account and machine boundaries. | +| **On-disk state** | The files ferry keeps under its state directory (`~/.local/state/ferry`) — the last-applied store, the agents target record, the backup journal, the immutable baseline, the restore and receive snapshots, and the work-domain state — plus the files `ferry work` writes into the configured cargo store, which cross account and machine boundaries. | ## The pre-1.0 rule @@ -42,9 +42,9 @@ From v1.0.0, a CLI or `ferry.toml` surface is removed only through deprecation: ## On-disk state versioning -Every state file ferry's domain machinery owns carries an integer `version` field -at the top level of its JSON. This lets a newer ferry recognise an older file and -a downgraded ferry recognise a file it cannot safely read. +Each state file in the envelope scheme carries an integer `version` field at the +top level of its JSON. This lets a newer ferry recognise an older file and a +downgraded ferry recognise a file it cannot safely read. The versioned files are: @@ -57,9 +57,18 @@ The versioned files are: | The cargo bundle's `ferry-work.json` manifest and the store's `claim..json` files | The manifest is written by `ferry work pack` (as a member of each `.ferrywork` bundle) and read by `ferry work receive`; the claim files are written by pack, receive, *and* take-back alike — each account appends only to its own `claim..json`, merged on read by receive and `work status`. These files routinely cross to a *different* account or machine, which makes their version envelope the most compatibility-relevant in ferry. | | `.abcd/.work.local/.ferry-handover.json` | The project-side handover marker pack records (never a cargo-store file); receive consumes and removes it. | -The immutable **baseline** (ferry's record of true pre-ferry state, which -`restore` reverses to) is a separate, content-addressed store and is read -version-independently; it is not part of this envelope scheme. +Two stores sit outside this envelope scheme and are read version-independently: + +- The immutable **baseline** — ferry's record of true pre-ferry state, which + `restore` reverses to — a separate, content-addressed store. +- The **snapshot store** (`snapshots//manifest.json`, plus one blob per + affected path), written before a `restore` mutates anything and by every + `ferry work receive`. + +Neither carries a `version` field, so the refuse-a-newer-file rule below does not +apply to snapshots: `restore --undo ` replays a snapshot without a version +gate, whichever version of ferry wrote it. That is why the snapshot manifest's +shape is treated as frozen. ### Reading an older file diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index b9f706c..21dbab5 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -179,19 +179,26 @@ surgery; the overlay is purely file-level. App-wide iTerm2 settings that live outside any profile are carried as an **allowlisted** `defaults` plist. Enable it with `iterm2 = true`. On `capture`, ferry exports the live `com.googlecode.iterm2` domain and keeps **only** an -allowlisted set of stable, machine-agnostic global keys (quit/close prompts, tab -and window chrome behaviour, dimming, clipboard behaviour, the auto-update -preference, the default-profile pointer). Everything else is dropped, so volatile +allowlisted set of stable, machine-agnostic global keys (quit/close prompts, +startup and window-restoration behaviour, tab and window chrome behaviour, +dimming, clipboard behaviour, mouse/pointer bindings, the auto-update preference, +the default-profile pointer). Everything else is dropped, so volatile machine state — window geometry, one-shot `NoSync…` dialog flags — can never reach the repo. The filtered plist is committed at `iterm2/com.googlecode.iterm2.plist`. -(The allowlist is a curated starting point; extend it in your own repo review.) +(The allowlist is compiled into ferry — `internal/terminal/allowlist.go` holds the +authoritative set of keys — and is curated rather than configured from the +manifest.) On `apply`, ferry imports that committed plist into the domain with `defaults import`, which **replaces** the whole `com.googlecode.iterm2` domain with the carried set — so any global key you have not allowlisted is reset. The dropped keys are the volatile ones (window geometry regenerates, `NoSync…` dialog -flags are one-shot), but if you rely on a global setting the starter allowlist -omits, add it to the allowlist so it is carried and preserved. +flags are one-shot), but if you rely on a global setting the allowlist omits, add +that key by hand to the committed `iterm2/com.googlecode.iterm2.plist` (or to the +`local/` overlay described below): `apply` imports the committed bytes verbatim, +so keys beyond the allowlist are carried and preserved. `capture` never re-adds +such a key, and an accepted whole-domain capture rewrites the file back to the +filtered set. **Quit iTerm2 first.** A running iTerm2 keeps its preferences in memory and rewrites the domain on quit, so mutating it while it runs is silently lost — both @@ -473,6 +480,13 @@ that belong to one machine only. `ferry capture` re-dumps the Brewfile from `brew bundle dump`; `ferry apply --deps` installs it with `brew bundle` (shared first, then the `.local` overlay). +The overlay is gitignored by the repos ferry creates — alongside +`ferry.local.toml` and `local/` — so it stays on the machine that wrote it and is +never synced, cloned, or bundled. Git's ignore rule covers untracked files only, +so an overlay that is already tracked stays tracked until +`git rm --cached deps/Brewfile..local` untracks it (the file itself stays on +disk); `git add -f` publishes one deliberately. + `ferry status` reports **Brewfile drift**: it compares the live `brew bundle dump` against the repo Brewfile and reports how many entries would be captured (installed locally but not recorded) or installed (recorded but not present). @@ -556,7 +570,9 @@ Some settings should differ per machine on purpose: a colour scheme on your lapt machine-specific tool. Those live in the `.local` layer: - **In the repo**: gitignored, under `local//` (e.g. `local/zsh/zshrc.local`, - `local/tmux/tmux.conf.local`, `local/git/gitconfig.local`). + `local/tmux/tmux.conf.local`, `local/git/gitconfig.local`). The dependency + overlay [`deps/Brewfile..local`](#homebrew) is gitignored the same way, + though it sits under `deps/` rather than `local/`. - **On the machine**: materialised to the real path (e.g. `~/.zshrc.local`, sourced last by the shared `~/.zshrc`; `~/.tmux.conf.local`, sourced last by the shared `~/.tmux.conf` via `source-file -q`; `~/.gitconfig.local`, pulled in last by the diff --git a/docs/tutorials/getting-started.md b/docs/tutorials/getting-started.md index e6d1b72..b121ff0 100644 --- a/docs/tutorials/getting-started.md +++ b/docs/tutorials/getting-started.md @@ -203,12 +203,20 @@ ferry apply --deps # install declared dependencies (needs brew and/or npm ``` `ferry apply` is idempotent and safe to re-run: run it after every `git pull`. It -never overwrites local edits you haven't captured: if a managed file has uncaptured -changes, `apply` reports a conflict instead of clobbering it. It also refuses to -replace a substantial existing file with an empty or blank repo source — that would -erase your config, so `apply` stops and names the file instead. Pass `--force` to -override (it warns and backs the file up first, so `ferry restore` can recover it), -or run `ferry capture` to save the current file into the repo before applying. +never overwrites local edits you haven't captured: a managed file with uncaptured +changes is left alone rather than clobbered — it is a capture candidate. If the repo +has changed that same file too, `apply` reports a conflict and refuses it until you +pick a winner. It also refuses to replace a substantial existing file with an empty +or blank repo source — that would erase your config, so `apply` stops and names the +file instead. Pass `--force` to override (it warns and backs the file up first, so +`ferry restore` can recover it), or run `ferry capture` to save the current file into +the repo before applying. + +ferry manages a single-branch config repo: `ferry sync` integrates and pushes `main` +and refuses any other checked-out branch. A repo whose default branch is named +something else needs renaming on the remote side as well — `git branch -M main`, +push that branch, and set it as the remote's default branch — because a local-only +rename leaves every other machine's clone on the old default. ## Move to another account or machine offline From 389628e8f72385d76d32d192bc03f304cc21aebc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 08:01:09 +0000 Subject: [PATCH 8/8] docs: changelog and decision log for bug-hunt round 9 Assisted-by: Claude:claude-fable-5 --- .abcd/work/DECISIONS.md | 1 + CHANGELOG.md | 105 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index d7f6095..1ce81b8 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -29,3 +29,4 @@ a `.abcd/development/decisions/` ADR when it shapes architecture or is expensive - 2026-08-08: Bug-hunt round 6 (fresh hunt, 4 dimensions, 4 hunters; 29 candidates, one independent adversarial refuter each — 9 confirmed, 20 refuted). Fixed five code/infra defects: a fresh config repo is pinned to the managed branch (bare `git init` honours the machine's `init.defaultBranch`, git's own default being `master`, while sync integrates and pushes `main` and refuses anything else — so `init --github` succeeded and every later `ferry sync` failed; the eval fixtures forced `-b main` instead of driving `ferry init`, so no test could see it); `init`/`bundle import` carry the machine-scoped fields across config.toml's wholesale rewrite (a plain `ferry init` re-run dropped `managed` and erased the `[work]` cargo store, silently un-configuring sync and every work verb — `[work]` now survives every route, `managed` only when the run reuses that repo); the pre-commit secret gate consumes a rename/copy origin field as parseStatusZ does (a 4-byte-or-longer origin was sliced as a status entry, so a staged `git mv src/dotfiles …` read as `/dotfiles`, hit EISDIR against the repo's own `dotfiles/` and fail-closed the whole sync on a path the user never touched); `appendLineOnce` normalises a missing trailing newline (an append onto an unterminated `.git/info/exclude` fused the two patterns — destroying the user's last ignore rule AND silently voiding ferry's — while scaffold printed success), and reports its close error; release.yml passes `--prerelease` for a suffixed tag (the tag gate deliberately admits one and both scripts skip cleanly so the run goes green, but the publish marked it a full release, making a rehearsal tag the repo's `latest` — the exact URL install.sh serves). Hardening: the two boundary-refusal message bodies move to internal/dotfile as `RefusalSSHBody`/`RefusalEscapeBody`, so the five plan packages and doctor's invariant matcher share one definition, plus a consistency-lint rule that fails on any re-inlined copy (a reword in one package silently turned a security-invariant [fail] into a [pass], with no compile error and nothing red). Docs: the tutorial's fresh flow stages with `add -A` before committing (`commit -am` never staged capture's new files, so the push published a repo missing the capture); the agents drift guidance names `ferry capture` first and drops the false "backed up, reversible" claim for `--force` (the write-once baseline holds the pre-ferry file, so restore cannot return a force-overwritten live edit). Decision log: four 2026-07-06 entries gained the `- ` marker they lacked, which made them CommonMark lazy continuations absorbed into the preceding bullet. Refuted as non-defects: the tutorial's "capture can push / apply can pull" purpose sentence (the same section attributes both to `ferry sync` 30 lines later, and the prerequisites table states the split up front); `.abcd/.work.local/` ignored via `.git/info/exclude` rather than `.gitignore` (ADR 0002 chooses this verbatim — "never `.gitignore`" — and release.sh returns early when NEXT.md is absent, so the claimed dirt chain cannot form); smoke-e2e.sh being CI-unwired (both assertions it claims as unique are covered more strictly by CI-run evals — restore byte-identity WITH mode in restore_test.go, and the ~/.ssh tripwire across the whole surface in safety_test.go plus a strace no-open gate); the auto-release three-failure bound counting transient failures (documented in cutting-a-release.md, recoverable by re-running, and strictly better than unbounded retry on an immutable tag); AGENTS.md:58 and CONTEXT.md:24 "repo-authoritative" (every clause is literally true — apply still skips drift — and configuration.md's emacs entry proves the formula carries no claim about capture, needing a separate "no capture pass" sentence); the plans' frontmatter divergence (check-plan-shipped.sh greps `Status:` and matches both forms; the README says "for example"); `ABCDevelopment` in six archival lines (AGENTS.md's rule is scoped to naming the AI assistant, the sibling `abcd-cli` repo is public, and scrubbing archival records is an owner call); five code nitpicks (restore's package count, Prune's partial report, the status.go errors.Is marker, config.toml's 0644, and .gitignore's create-only perm) and nine infra/docs nitpicks (auto-merge timeout — reusable-workflow calls cannot take one either, so the premise was false; the actions:read comment; status=failure under-counting, which loosens not tightens; the START_SHA tripwire, which is documented fail-closed; the public/private comments; the CLI-page naming column, whose two-level pattern reads correctly; the drift page's link blurb; the .work.local link, deliberate and self-describing; ADR 0003's heading depth, which MADR permits). - 2026-08-13: Bug-hunt round 7 (fresh hunt, 4 dimensions, 4 hunters; 23 unique candidates, one independent adversarial refuter each — 19 confirmed, 4 refuted). Fixed nine substantive defects: every work verb refuses a cargo store that is or resolves into `~/.ssh` before any read/write on the path (the store was the one configurable, ferry-written path with no ssh guard — `work pack` created a world-writable directory inside `~/.ssh` and wrote bundles there, while the guarded repo/`--out` paths prove hand-edited config is in the threat model); the pre-commit secret gate walks a collapsed untracked directory (`?? dir/` at default -unormal EISDIR'd the fail-closed read, wedging every sync after a capture created a new repo subdirectory, with advice a directory can never satisfy — the sibling backup pass already walked exactly this shape, and the round-3 test dodged it with a staged `add -A`); `ferry init ` wiring the already-configured repo carries `managed` (the round-6 carry keyed on the no-arg route, so `ferry init .` from inside the configured repo dropped the flag with no supported way back — `init --github` refuses while a repo is configured; the carry is gated on the route having WIRED a pre-existing worktree, because a clone or fresh seed can land on the recorded path after the old repo was deleted and must never inherit the flag — the final dual review caught exactly that hole in this round's first cut, both reviewers NO-SHIP, fixed in the single remediation round with an eval pinning the recreated-at-same-path case); `bundle import` pins HEAD to the managed branch before its initial commit (the same unpinned `git init` round 6 fixed for the fresh path, left on the import route); `init --github`'s success banner and existing-repo hint name `ferry sync` as the publish verb (the flow's final instruction claimed capture pushes and apply pulls — capture never commits or pushes, apply never touches the network, and the banner is the last thing the flow prints); drift/conflict guidance is keyed on the registry's Captures() flag, so status stops steering terminals/keybindings/emacs/iterm2-profiles at `ferry capture` and iterm2-profiles stops falling through apply/diff's hand-maintained domain list to the dotfiles wording (an existing eval forbade exactly this wording for diff but never ran status); `apply --deps` states when a failed installed-set snapshot suppressed the restore-record (the fail-closed suppression printed the same "installed 0 package(s)" as a benign re-run, and restore --packages silently lost the run); release.yml asserts the built binary reports the tag before checksumming (Go's linker silently drops -X for a moved symbol, so a rename would publish -dev-reporting binaries with valid checksums and provenance, nothing red) and verifies the published assets against the published checksums.txt after the release (the pairing install.sh depends on; runs on private repos where the attestation check is gated off); consistency-lint invariant 5 fails if `.abcd/.work.local/` is ever tracked, and `make preflight` runs the lint so the pre-push hook catches it before publication (the exclude rule is per-clone and absent in a fresh checkout, so `git add -A` staged the private tier with every gate green — ADR 0002's exclude-not-gitignore choice honoured, enforcement added where none existed). Docs: the compatibility contract covers the four work-domain versioned files (including the machine-crossing cargo manifest) and states their unversioned-is-refused rule plus the no-retention policy for journal runs and snapshots; README/tutorial scope the "never requires sudo" claim to name the apt `apply --deps` exception; doctor's synopsis says git (required) vs zsh/package manager (recommended); install.sh documents FERRY_VERSION and its pipe-placement footgun; evals README lists init_preserve_test.go; the drift page's remedy bullet covers all four repo-authoritative domains; the stale lint step labels name all the invariants; prune-releases refuses to prune when the just-published release is missing from a listing whose fetch failure the empty-grep guard would otherwise swallow. Refuted as non-defects: Brewfile inline-comment rejection (CHANGELOG documents the fail-closed trade-off verbatim, and comment-stripping would regress a legal `#` inside an option value); the release verify 20-minute timeout (measured runs finish in ~2 minutes — 9x headroom — and the burnt-version consequence is documented and one-click recoverable); the refusal-constant concatenation (vet folds constant concatenation and the const-ness is compile-enforced); FERRY_NO_NETWORK's absent header doc (a contract-optional test hook that installs nothing). Carried forward: capture's own completion hint never names `ferry sync` for a managed repo; installApt/installBrew swallow the package manager's stderr (exit-status-only errors); the round-6 `carryMachineScoped` sites now have eval coverage but the unit seam stays untested; work-domain state growth has no pruner (documented this round as deliberate for snapshots; journal runs are inert dead bytes a future keep-last-N sweep could clear); guided_apply buckets the repo-authoritative domains under a "dotfiles" label. - 2026-08-14: Bug-hunt round 8 (fresh hunt, 4 dimensions, 4 hunters; 22 unique candidates, one independent adversarial refuter each — 12 confirmed, 12 refuted, 2 hunter-self-refuted). Fixed six code defects: guided apply's risky walkthrough groups every FileDomain under its own name in registry order (a two-name literal switch predating fn-5 dropped keybindings/emacs/iterm2-profiles into the "dotfiles" bucket, so the group header and the bulk consent named a domain the user was not reviewing and one "yes" spanned the mixed bucket); capture's brew/npm re-dumps count a change only when the manifest's bytes actually changed (`brew bundle dump --force` rewrites unconditionally, so every clean re-run claimed "wrote 1 change(s)" over an empty `git status` and "nothing has drifted" was unreachable on any machine managing either domain), with the npm list written via backup.AtomicWrite and left untouched when identical; capture's terminal preference-domain compare resolves the local overlay first via terminalRepoStatusSource, exactly as apply/status/diff do (a domain captured to [l]ocal was re-offered as drifted forever, and re-accepting it shared would promote machine-divergent settings); the deps install/dump rails carry the package manager's combined output in their errors as the uninstall rail already did (a failed apply --deps surfaced as a bare "exit status 1"); backup.Engine snapshots run the resolved-parent containment guard inside snapshotCurrent so the exported Snapshot used by work receive is guarded identically to restore (a parent symlink out of $HOME let the pre-receive snapshot read and persist out-of-home content the write boundary then refused); the release version-stamp gate compares exactly against "ferry " in release.yml and release.sh (the dev line is next-release-plus--dev, so the substring grep passed an unstamped binary on precisely the modal release). Docs: the README/tutorial "one privileged step" claim covers restore --packages (the apt uninstall rail runs apt-get as root, per the code's own comments, and no user-facing page said so); the compatibility contract's claim-file row names pack, receive, and take-back as writers (receive.AppendClaim writes claim..json); the commands reference gains the agents parent-noun row; make preflight enters AGENTS.md/CONTRIBUTING.md; the consistency lint's private-tier advice mandates amending committed history (git rm --cached alone greens the gate while the push still publishes). Refuted as non-defects: the terminal-capture placeholder plaintext leak (placeholders exist only for content GateValue already rates High, and the re-export re-trips the gate); the hand-over sudo mkdir (round-1 adjudication stands — a host-OS setup example, not a ferry operation); the release how-to's five-step summary (explicitly non-exhaustive by its own structure); +build-metadata tags becoming latest (SemVer-correct and deliberate per the workflow's own comment); the attestation docs' missing visibility caveat (true of everything a reader can download); the retention-policy scoping, [work]-in-ferry.toml, topic-plan lifecycle, behavioral-in-evals-README, "works today" framing, attribution-rule carve-out (rule already scoped to credit; amending owner policy is an owner call), the pre-push hook's absent in-repo installation (dispatcher is deliberately external; hazard population equals hook population), and a range-based invariant-5 check (breaks CI's detached checkout — the advice-text fix shipped instead). Merge-gate remediation (one round, regressions reviewer's blocker): commands.md's apply row still enumerated the walkthrough groups as "dotfiles / agents / terminals" — the exact list the code fix deleted — now naming all six domains; folded in the reviewers' safe notes: the status drift dump was a fifth output-discarding site (internal/deps/status.go), a dump that creates an empty manifest now counts as a change (nil-vs-empty bytes compare equal, so existence is checked), groupRisky gained the unit tests it shipped without, the dump.go comment and capture's "exactly as apply" claim were corrected, and the CHANGELOG narrates the npm 0644 normalisation and receive's abort-first failure mode. Carried forward: capture's completion hint still never names ferry sync for a managed repo (adjudicated a nitpick, unfixed this round); the managed carry stays path-keyed (an origin-URL comparison would close it); terminalRepoStatusSource ignores an extensionless local overlay that apply's terminalExportBlob honours, and falls back to shared behind a poisoned overlay where apply fails closed (pre-existing, inherited by capture's compare); guided-apply's per-domain groups have no direct interactive eval; the darwin-only defaults export has no injectable seam, so the terminal-capture fix is pinned at the terminalRepoStatusSource seam rather than end-to-end. +- 2026-08-15: Bug-hunt round 9 (fresh hunt, 4 dimensions, 4 hunters; 36 candidates, one independent adversarial refuter per finding with same-site findings grouped — 28 confirmed, 8 refuted/rejected). Fixed thirteen substantive defects: the pre-commit secret scan Lstat-gates every changed path (a symlink-following read pulled `~/.ssh/id_ed25519` into the process on the default sync path — a direct boundary breach — and a dirty submodule or symlink-to-directory EISDIR-wedged every sync with unsatisfiable advice); the collapsed-untracked-directory scan enumerates via `git ls-files --others --exclude-standard` so gitignored files and nested `.git` trees no longer false-block a sync `git add -A` would never publish; a shared terminal capture removes the per-machine overlay that would otherwise shadow it forever (status/apply/capture disagreed indefinitely, re-offering the domain on every run); status and capture render `{{ferry.secret …}}` placeholders through the store before comparing terminal domains — with the placeholder write made a byte-exact round-trip — so a secret-routed capture converges instead of reporting permanent drift while apply says in-sync; `npm ls -g` is parsed from stdout alone with stderr spliced into real errors (CombinedOutput fusion made the documented non-zero-exit tolerance dead code, disabling the domain on any peer-dep warning); `ferry diff` predicts the empty-over-substantial refusal as "would refuse" and an aborted apply states its rollback (the guard's abort policy itself adjudicated deliberate and untouched); repos ferry creates gitignore `deps/Brewfile.*.local` on every init route with the `--github` gate model rendered from the writer's own pattern set (the documented per-machine overlay was committed, synced, installed fleet-wide, and bundled); release publishing is idempotent and auto-release's heal predicate requires a published, asset-complete release (a red post-publish check was permanently un-re-runnable while a full re-run went green with the bypassed gates unrun); the by-hand checksum recipe is the single `make checksums VERSION=` invocation (the two-line form silently re-built dev-stamped binaries under a self-consistent manifest); the compatibility contract names restore snapshots as the second version-independent store (the refuse-newer rule never gated `restore --undo`); configuration.md stops directing users to "extend" the compiled-in iTerm2 allowlist and documents the committed-plist route that actually works; the single-branch `main` constraint on sync enters the reference, tutorial, and help text. Nitpicks fixed: conflict items in the guided walkthrough say "yes" will not overwrite them; clean skip-always targets are silent and counted in-sync; the receive unlock accumulates releases (latent double-lock leak); consistency-lint invariant 3 exempts only test files; zizmor's SARIF upload skips fork PRs (audit still gates); install.sh names a failed binary download; ci.yml's cross-compile comment stops claiming a pre-tag gate it is not (auto-release tags the same push in ~20s); the release how-to documents step 7's local NEXT.md reset; allowlist categories, route-jargon, ssh.md's enumeration, scaffold help, AGENTS.md's CI list, and the tutorial's drift-vs-conflict wording corrected. Refuted as non-defects: restore --packages count (unreachable via ferry-written state); auto-merge retitle disarm (same end state as the round-4/6 refutation, one-admin actor set, disarm step racy and clobbers hand-armed PRs); the green terminal-state annotation (three red runs already notified; a transition-fail fires unreliably on a non-monotonic two-counter sum); +metadata tags as latest (round-8 adjudication stands — the workflow comment weighs the latest/install.sh consequence explicitly, retention-escape applies to every suffixed tag by documented design); stale-binary checksums (local rehearsal file only, gitignored); the doubled push/pull_request CI runs (different trees, and a shared group would cancel required checks); the wizard interactive fallback (disclosed twice on the same doc page, conservative and secret-safe by design — Codex M3 chose fallback over error for redirected runs; wording tightened anyway). Carried forward: terminalRepoStatusSource's extensionless/poisoned-overlay divergences from terminalExportBlob (hand-authored state only; one shared resolver would close it); Apple Terminal's unfiltered export routinely trips the entropy gate on NSKeyedArchiver base64, possibly making that domain uncapturable on customised machines (new lead from the refutation pass, unadjudicated); the deps overlay glob is duplicated in cmd/init.go rather than shared from internal/deps; release.sh's render_next_md "also sourced by tests" claim matches nothing, and a NEXT.md missing its carry markers makes step 7 exit the driver non-zero after a successful tag push; the heal predicate hardcodes the 5-asset count; sync's branch-rename hint still omits the remote-side default-branch step; machine-config `managed` remains path-keyed. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b582c4..81d2f0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,111 @@ called out in a **Breaking** section. See ### Fixed +- **The pre-commit secret scan never reads through a repo symlink.** + `ferry sync`'s changed-file scan opened paths with a symlink-following + read, so an untracked or modified symlink in the config repo — for + example one pointing at `~/.ssh/id_ed25519` — was read in full, + breaching the `~/.ssh` boundary and false-blocking the sync. Changed + paths are now Lstat-gated and non-regular entries are skipped: git + commits a symlink as its link text, which the push-range blob scan + already covers. +- **`ferry sync` no longer wedges on directory-shaped status entries.** + A dirty submodule or an untracked symlink-to-directory arrives without + the trailing slash the scan used to spot directories, so the fail-closed + read aborted every sync with advice ("re-run once the file is readable") + a directory can never satisfy. Such entries are skipped — a gitlink's + content never enters the push range. +- **The untracked-directory secret scan matches what `git add -A` + stages.** The walk over a collapsed untracked directory read every file + beneath it, including gitignored files and nested repositories' `.git` + internals, so a token in a file git would never commit blocked the sync + with no way forward. The directory is now enumerated with + `git ls-files --others --exclude-standard`, scanning exactly the + stageable set; a failed enumeration still aborts. +- **A shared terminal capture behind a local overlay converges.** + Accepting a terminal preference domain to shared wrote the shared + plist, but the per-machine overlay from an earlier local capture kept + winning both the drift comparison and `apply`, so the domain was + re-offered forever and the shared bytes did nothing. A shared accept + now removes the superseded overlay and says so; a symlinked overlay is + left untouched. +- **A secret-routed terminal capture stops reporting permanent drift.** + Capture writes a `{{ferry.secret …}}` placeholder into the repo plist, + but `status` and `capture` compared the raw placeholder bytes against + the live export, so the domain read as drifted forever — and re-prompted + the secret gate on every capture — while `apply`, which renders + placeholders, considered it in sync. Both now render the repo side + through the secret store before comparing (falling back to the raw + compare when the store or ref is unavailable), and the placeholder file + round-trips byte-exactly. +- **Peer-dependency warnings no longer disable the npm-globals domain.** + `npm ls -g` writes diagnostics to stderr while still emitting the full + JSON tree on stdout; the dump parsed the two streams fused, so any + non-zero exit was a hard error despite the documented tolerance. The + listing is parsed from stdout alone, and a genuine failure carries + npm's own stderr in the error instead of a bare exit status. +- **`ferry diff` predicts the empty-over-substantial refusal.** The + data-loss guard that aborts `apply` when a near-empty repo file would + replace a substantial live file was invisible to the preview, which + showed a plain "would update". The plan renders such items as "would + refuse" and counts them in the summary, and when the guard does abort + an apply, a closing notice states that the changes reported earlier in + the run were rolled back. +- **Repos ferry creates gitignore the per-machine dependency overlay.** + `deps/Brewfile..local` is documented as belonging to one machine + only, but the generated `.gitignore` covered only `ferry.local.toml` + and `local/`, so `ferry sync` committed the overlay, every other + machine installed it via `apply --deps`, and `bundle export` carried + it. The ignore list covers the overlay on every init route, with the + `init --github` pre-create gate model rendered from the same pattern + set. An overlay committed before the rule existed stays tracked until + `git rm --cached` untracks it — documented, alongside the deliberate + `git add -f` escape, in the configuration reference. +- **A red release run after publishing is recoverable.** The publish + step errored on re-run once the Release existed, so a failed + post-publish check (asset verification, retention prune) left the run + permanently red — while re-running the whole auto-release run saw the + release as done and quietly skipped the bypassed checks. Publishing is + idempotent (a re-run re-uploads the assets and re-asserts the release + metadata), and auto-release treats a version as released only when its + Release is published and carries the full asset set. The release + how-to documents post-publish recovery and corrects the claim that a + failed check fails the release. +- **The by-hand checksum recipe stamps the version.** The documented + `make build VERSION=…` + `make checksums` pair silently rebuilt the + binaries without the version stamp before hashing (the `checksums` + target's build prerequisite is phony and always re-runs), so a + hand-published release shipped dev-stamped binaries under a + self-consistent manifest. The recipe is the single + `make checksums VERSION=vX.Y.Z` invocation, and the Makefile comment + states the rebuild behaviour. +- **`install.sh` reports a failed binary download.** The download ran + silenced under `set -e`, so a missing release asset ended the install + with no output after the "downloading…" line; it fails with a message + naming the asset. +- **CI's workflow audit passes on fork pull requests.** The SARIF + upload needs a writable token, which a fork PR never gets; the upload + is skipped there while the audit itself still runs and gates. +- **Guided apply is truthful about conflicts and skip-always targets.** + The walkthrough prompted "yes to apply" for conflict items it never + overwrites; the listing and details view now state that confirming + does not overwrite a conflict (`ferry capture` or `apply --force` + resolves it), and the commands reference lists the conflict as the + fourth risky class. A clean skip-always target no longer prints a + skip line on every run and is counted as in sync. +- **Docs corrected against the code.** The compatibility contract names + restore snapshots as the second version-independent store (the + refuse-a-newer-file rule does not gate `restore --undo`); the + configuration reference states the iTerm2 global allowlist is compiled + into ferry, lists all eight kept categories, and gives the committed- + plist route for carrying extra keys; the single-branch `main` + constraint on `ferry sync` is stated in the reference, the tutorial, + and the command help; "route-1/route-2" jargon is replaced with plain + language; ssh.md states the `~/.ssh` invariant universally including + the cargo-store guard; the scaffold help names exactly what it + creates; AGENTS.md's CI list names the gitleaks and zizmor gates; the + tutorial distinguishes local drift from a conflict. + - **A cargo store under `~/.ssh` is refused.** The `[work] store` path is hand-configured, and every other configurable path ferry writes through — the repo path, `bundle import --out` — is guarded against resolving into `~/.ssh`