From 4568ea3adfdedf7b09738e0f2015b437074f5e93 Mon Sep 17 00:00:00 2001 From: Artem Matskevych <52060443+Reederey87@users.noreply.github.com> Date: Mon, 6 Jul 2026 06:52:55 -0400 Subject: [PATCH] feat(worktree): cleanup reaps squash/rebase-merged worktrees (P4-GIT-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit worktree cleanup --merged only tested ancestry, so squash- and rebase-merged branches were skipped forever. New git.Runner.IsSquashMerged uses a current-tree merge probe (git merge-tree --write-tree, git >= 2.38): merged iff the simulated merge's tree equals the current base tree. The first-draft git cherry/patch-id chain was replaced in dual review — historical patch-id equivalence matches merged-then-reverted changes and would reap unmerged work. Conservative on conflict/error; the inherent coincidental-identical-diff limitation is tripwire-pinned and every reap prints the deleted branch's tip SHA as the recovery breadcrumb. Base fetched under the repo lock (deduped per project/ref); reaped branches get git branch -D. Ledger: P4-GIT-04 -> Recently shipped. Co-Authored-By: Claude Fable 5 --- docs/audits/README.md | 3 +- internal/cli/worktree.go | 48 ++++++- internal/cli/worktree_test.go | 121 +++++++++++++++++ internal/git/git.go | 42 ++++++ internal/git/git_test.go | 136 +++++++++++++++++++ spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md | 4 + spec/13_CLI_DAEMON_API.md | 2 +- spec/16_TEST_PLAN.md | 2 +- spec/18_WORK_LOG.md | 14 ++ 9 files changed, 367 insertions(+), 5 deletions(-) diff --git a/docs/audits/README.md b/docs/audits/README.md index 178b419..4e915ac 100644 --- a/docs/audits/README.md +++ b/docs/audits/README.md @@ -94,6 +94,7 @@ Currently-actionable findings, pass-scoped. Earlier passes (1–3) are largely i | P4-QUAL-02 | P1 | `feat/p4-qual-02-properties` (2026-07-04) | Property/model-check layer over the pure `Decide`/`Projection` seam (`P5-ARCH-01`→PR #87→here), on the test-only `pgregory.net/rapid` dep (zero transitive deps, adopted per this finding's explicit ask). Four rapid suites + a fuzz bridge: (1) `hlc_property_test.go` — Send/Receive interleavings under a backward-stepping INJECTED clock prove strict Send monotonicity, Receive non-regression, the EXACT `MaxSkew` accept/reject boundary, and logical-overflow carry; (2) `decide_rapid_test.go` — randomized convergence: two independent delivery permutations of a generated event set fold to one `Projection` + duplicate idempotency, wrapped as coverage-guided `FuzzDecideConvergence` (`go test -fuzz`; a 30s ubuntu CI step); (3) `import_replay_property_test.go` — `BuildSnapshot`→`ImportSnapshot` then subset-replay ≡ full replay (active rows); (4) `replica_model_test.go` — the audit's 3-replica model check: independent orders SPLIT into sequential `ApplyEvents` batches (cross-pull-window realism, where the pre-#87 divergence hid) converge byte-identically, with duplicate re-delivery (idempotency) and a tombstone-GC interleaving (active-rows-only invariant, since GC purges only deleted rows). The generator is confined to the provably-convergent class by TWO witness-pinned exclusions. **New finding surfaced here** (beyond P5-ARCH-01's documented delete residual): same-path/different-remote convergence is order-dependent **with NO delete involved** whenever one remote carries multiple events — same-remote LWW keeps that remote's HIGHEST HLC while cross-remote `reconcileSamePath` keeps the LOWEST coordinate, so the winner flips by delivery order. Both divergences share the lowest-coordinate reconcile root cause and are pinned by `TestDecideDifferentRemote{Delete,MultiEvent}DivergesWitness` tripwires that FAIL the day `reconcileSamePath` is made LWW-consistent, forcing the matching generator exclusion to be removed. **Follow-up SHIPPED 2026-07-04** (`fix/reconcile-same-path-hlc-monotonic`): the winner is HLC-monotonic (highest `(HLC, deviceID, eventID)`, consistent with same-remote LWW and `importEntryTx`); both tripwires fired as designed and were retired with their exclusions, and `genEventSet` now draws the full event space (deletes + multi-event remotes across different-remote paths). | | P6-XP-03 | P2 | `fix/p6-xp-03` (2026-07-04) | `run-loop` now runs its advertised **scan** stage (chosen over the doc-only fix): `runLoopTick` prepends an idempotent scan+adopt before `runSyncCycle`, restoring the local→hub auto-path the daemonless loop otherwise lacked. Idempotency (the hazard: `adoptFindings` unconditionally emits `project.added` per finding) is handled by `findingAlreadyAdopted` — a finding is adopted only when `store.ProjectByPath` has no active row matching its type and (for `git_repo`) `remote_key`; `adoptNewFindings` filters then delegates to the existing `adoptFindings`, so one-shot `scan --adopt` is unchanged. Warning-class findings (secret-looking files, symlink escapes) and duplicate-remote findings are routed to stderr and NEVER auto-adopted (duplicates are dropped in the loop). The stderr tick header + `Short`/doc/README/spec-00 wording are reconciled to "scan + sync + materialize." Pinned by `TestRunLoopScanAdoptIdempotentAndPicksUpNewRepos` (4 ticks, no duplicate events + mid-run pickup), `...SkipsDuplicateRemotes`, `...WarnsSecretWithoutAdopting`, and an extended `run_loop_once.txtar` (`pushed 2 → pushed 0`). | | P6-DATA-04 | P2 | `fix/p6-data-04` (2026-07-04) | `devstrap db backup --full ` now captures a recoverable workspace: `state.db` (`VACUUM INTO`), the `blobs/.age` files `AllBlobRefs` reports, the key material (`keys/` under file custody; a custody-aware keychain **escrow** — device age + Ed25519 signing identities + every held WCK epoch from `HeldKeys` + hub S3 creds via `HybridStore.ExportForBackup`, a hard error if any required key is unreadable), AND `config.yaml` (the hub pointer + custom root). New `devstrap db restore ` refuses a non-empty state dir without `--force`, validates the staged `state.db` (`quick_check`+`foreign_key_check`) before promoting, and swaps ONLY the captured targets in place (temp+rename+rollback) so un-captured Home data (`quarantine/`, `logs/`) survives; a zip-slip guard (`sanitizeBackupEntry`) rejects abs/`..`/symlink/out-of-layout entries. New doctor **dangling-blob-refs** check + a keychain-custody restore warning; the `doctor` "restore from a db backup" remedy now points at `--full`. Round-trip test proves `env capture → backup --full → wipe → restore → env hydrate` recovers the identical secret AND config `hub`/`root` survive; blocking review finding (config.yaml omission / whole-Home wipe) fixed. spec/12 disaster-recovery runbook. | +| P4-GIT-04 | P2 | `feat/p4-git-04-worktree-gc` (2026-07-06) | `worktree cleanup --merged` now also reaps squash/rebase-merged worktrees: new `git.Runner.IsSquashMerged` — offline content-equivalence via the current-tree merge probe (`git merge-tree --write-tree `, git ≥ 2.38: merged iff the simulated merge's tree equals the CURRENT base tree). The first-draft `git cherry`/patch-id chain was replaced in dual review — historical patch-id equivalence matches merged-then-REVERTED changes and would reap unmerged work (both reviewers reproduced it). Conservative rule: conflicting merge / old git / any error = not merged. Documented accepted limitation: a coincidentally-identical unrelated commit reads as merged (tripwire-pinned); every reap prints the deleted branch's tip SHA as the recovery breadcrumb. Ancestry stays the first check; misses consult content-equivalence, labeled `merged (squash)`; base fetched under the repo lock, deduped per project/ref; reaped branches get `git branch -D` (warn-only). Forge-API (`gh pr list`) cross-checks are an explicit non-goal — cleanup works offline. Pinned by the real-git `TestIsSquashMerged*` matrix (incl. revert-on-base) + CLI e2e `TestWorktreeCleanupReapsSquashMergedWorktree`. | ### Pass 6 (2026-07-01) — 0 open of 43 — **PASS CLOSED**; **all five P1s shipped**; **quick-win, sync-convergence, and snapshot-exchange waves complete** @@ -161,7 +162,7 @@ Currently-actionable findings, pass-scoped. Earlier passes (1–3) are largely i | P4-HUB-15 | P2 | Cost controls, quotas, rate limiting | | P4-HUB-16 | P2 | At-rest versioning/Object-Lock + backup/replication runbook | | P4-GIT-03 | — | **Shipped 2026-07-05** — OS-enforced agent sandbox complete: macOS Seatbelt + Linux bubblewrap + Linux Landlock fallback (write confinement, credential denies, policy-scoped network deny), the Linux seccomp syscall denylist, unsigned local `sandbox.violation` telemetry, and tighter read confinement (`--read-confine`, default-on for `readonly`) — moved to _Recently shipped_ above. The three named remaining items (seccomp, `sandbox.violation` telemetry, tighter read confinement) are all shipped; containerization stays a separate spec/14 roadmap direction, not a `P4-GIT-03` remainder. | -| P4-GIT-04 | P2 | Worktree GC that reaps squash/rebase-merged worktrees | +| P4-GIT-04 | — | **Shipped 2026-07-06** (`feat/p4-git-04-worktree-gc`) — moved to _Recently shipped_ above. | | P4-GIT-07 | P3 | Persisted per-project materialize failure record, resume, progress detail | | P4-QUAL-04 | P2 | Enforce coverage in CI + add a Windows build _(narrowed 2026-07-05: the coverage half shipped — ci.yml generates `coverage.out` and gates a 50% `go tool cover -func` floor, closing `P5-QUAL-04`; remaining open scope is the Windows build only — `internal/platform` build tags are already windows-clean, known runtime risk: the `syscall.Signal(0)` liveness probe in `internal/cli/repo_lock.go`)_ | | P4-QUAL-05 | — | **Shipped v0.1.1 (2026-07-05)** — moved to _Recently shipped_ above. | diff --git a/internal/cli/worktree.go b/internal/cli/worktree.go index adc0e67..43524a8 100644 --- a/internal/cli/worktree.go +++ b/internal/cli/worktree.go @@ -474,6 +474,10 @@ func newWorktreeCleanupCommand(stdout io.Writer, opts *options) *cobra.Command { } removed := 0 skipped := 0 + // One base fetch per (project, base ref) per run — N worktrees on + // the same project must not trigger N redundant network fetches + // (review finding). + refreshed := map[string]bool{} for _, wt := range worktrees { r := gitRunner(opts) project, err := store.ProjectByID(cmd.Context(), wt.NamespaceID) @@ -511,19 +515,46 @@ func newWorktreeCleanupCommand(stdout io.Writer, opts *options) *cobra.Command { skipped++ continue } + if refreshKey := project.ID + "\x00" + wt.BaseRef; !refreshed[refreshKey] { + refreshed[refreshKey] = true + if err := refreshWorktreeBase(cmd.Context(), opts, r, project, repoPath, wt.BaseRef); err != nil { + _, _ = fmt.Fprintf(stdout, "warning: could not refresh %s for worktree %s: %v; using local ref\n", wt.BaseRef, wt.ID, err) + } + } + mergeLabel := "merged" mergedOut, err := r.Run(cmd.Context(), wt.Path, "branch", "--merged", wt.BaseRef, "--list", wt.Branch) if err != nil || !strings.Contains(mergedOut, wt.Branch) { - skipped++ - continue + squashMerged, squashErr := r.IsSquashMerged(cmd.Context(), wt.Path, wt.Branch, wt.BaseRef) + if squashErr != nil || !squashMerged { + skipped++ + continue + } + mergeLabel = "merged (squash)" + } + // Recovery breadcrumb: content-equivalence can match a + // coincidentally-identical unrelated commit (documented + // limitation), so name the deleted branch's tip — recreating + // it is one `git branch ` away until git gc. + tip := "" + if out, terr := r.RevParse(cmd.Context(), repoPath, wt.Branch); terr == nil { + tip = strings.TrimSpace(out) } if err := r.WorktreeRemove(cmd.Context(), repoPath, wt.Path, force); err != nil { skipped++ continue } + if _, err := r.Run(cmd.Context(), repoPath, "branch", "-D", wt.Branch); err != nil { + _, _ = fmt.Fprintf(stdout, "warning: failed to delete branch %s for removed worktree %s: %v\n", wt.Branch, wt.ID, err) + } if err := store.MarkWorktreeRemoved(cmd.Context(), wt.ID); err != nil { return err } removed++ + if tip != "" { + _, _ = fmt.Fprintf(stdout, "Removed worktree %s (%s; branch %s was at %s)\n", wt.ID, mergeLabel, wt.Branch, tip) + } else { + _, _ = fmt.Fprintf(stdout, "Removed worktree %s (%s)\n", wt.ID, mergeLabel) + } } _, err = fmt.Fprintf(stdout, "Cleaned up %d worktrees (%d skipped)\n", removed, skipped) return err @@ -534,6 +565,19 @@ func newWorktreeCleanupCommand(stdout io.Writer, opts *options) *cobra.Command { return cmd } +func refreshWorktreeBase(ctx context.Context, opts *options, r dsgit.Runner, project state.ProjectStatus, repoPath, baseRef string) error { + remote, branch, ok := strings.Cut(baseRef, "/") + if !ok || remote == "" || branch == "" { + return fmt.Errorf("base ref must be remote/branch, got %q", baseRef) + } + unlock, err := acquireRepoLock(opts.paths().Home, project.ID) + if err != nil { + return err + } + defer unlock() + return r.Fetch(ctx, repoPath, remote, branch) +} + var slugPattern = regexp.MustCompile(`[^a-z0-9]+`) const worktreeBranchAttempts = 3 diff --git a/internal/cli/worktree_test.go b/internal/cli/worktree_test.go index 8eb0942..62ca8ab 100644 --- a/internal/cli/worktree_test.go +++ b/internal/cli/worktree_test.go @@ -3,6 +3,7 @@ package cli import ( "context" "database/sql" + "encoding/json" "errors" "fmt" "net/url" @@ -108,6 +109,89 @@ func TestCreateFreshWorktreeCleansUpAfterInsertWorktreeFailure(t *testing.T) { assertNoAgentBranches(t, localPath) } +func TestWorktreeCleanupReapsSquashMergedWorktree(t *testing.T) { + home := filepath.Join(t.TempDir(), ".devstrap") + root := filepath.Join(t.TempDir(), "Code") + localPath := setupFreshWorktreeRepo(t, home, root, "auto", false) + // Repo-local identity is shared by the clone's worktrees; CI runners have + // no global git config, so the worktree commits below need it. + runGit(t, localPath, "config", "user.email", "devstrap@example.test") + runGit(t, localPath, "config", "user.name", "DevStrap Test") + + stdout, stderr, err := executeForTest("--home", home, "--root", root, "worktree", "new", "work/acme/repo", "--fresh-upstream", "--name", "squash merged") + if err != nil { + t.Fatalf("worktree new squash stdout = %q stderr = %q err = %v", stdout, stderr, err) + } + stdout, stderr, err = executeForTest("--home", home, "--root", root, "worktree", "new", "work/acme/repo", "--fresh-upstream", "--name", "unmerged") + if err != nil { + t.Fatalf("worktree new unmerged stdout = %q stderr = %q err = %v", stdout, stderr, err) + } + worktrees := listWorktreesForTest(t, home, root) + if len(worktrees) != 2 { + t.Fatalf("worktree count = %d, want 2: %+v", len(worktrees), worktrees) + } + var squashWT, unmergedWT testWorktree + for _, wt := range worktrees { + switch { + case strings.Contains(wt.Branch, "squash-merged"): + squashWT = wt + case strings.Contains(wt.Branch, "unmerged"): + unmergedWT = wt + } + } + if squashWT.ID == "" || unmergedWT.ID == "" { + t.Fatalf("did not find expected worktrees: %+v", worktrees) + } + + if err := os.WriteFile(filepath.Join(squashWT.Path, "squashed.txt"), []byte("squashed\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit(t, squashWT.Path, "add", "squashed.txt") + runGit(t, squashWT.Path, "commit", "-m", "squashed change") + runGit(t, localPath, "push", "origin", squashWT.Branch) + + if err := os.WriteFile(filepath.Join(unmergedWT.Path, "unmerged.txt"), []byte("unmerged\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit(t, unmergedWT.Path, "add", "unmerged.txt") + runGit(t, unmergedWT.Path, "commit", "-m", "unmerged change") + + tmp := t.TempDir() + remote := strings.TrimSpace(runGitOutput(t, localPath, "remote", "get-url", "origin")) + integrator := filepath.Join(tmp, "integrator") + runGit(t, tmp, "clone", remote, integrator) + runGit(t, integrator, "config", "user.email", "devstrap@example.test") + runGit(t, integrator, "config", "user.name", "DevStrap Test") + runGit(t, integrator, "checkout", "main") + runGit(t, integrator, "fetch", "origin", squashWT.Branch) + runGit(t, integrator, "merge", "--squash", "origin/"+squashWT.Branch) + runGit(t, integrator, "commit", "-m", "squash merge worktree") + runGit(t, integrator, "push", "origin", "main") + + stdout, stderr, err = executeForTest("--home", home, "--root", root, "worktree", "cleanup", "--merged") + if err != nil { + t.Fatalf("cleanup stdout = %q stderr = %q err = %v", stdout, stderr, err) + } + if !strings.Contains(stdout, "merged (squash)") { + t.Fatalf("cleanup stdout = %q, want squash merge label", stdout) + } + if !strings.Contains(stdout, "Cleaned up 1 worktrees (1 skipped)") { + t.Fatalf("cleanup stdout = %q, want one removed and one skipped", stdout) + } + if got := worktreeStatusForTest(t, filepath.Join(home, "state.db"), squashWT.ID); got != "removed" { + t.Fatalf("squash worktree status = %q, want removed", got) + } + if got := worktreeStatusForTest(t, filepath.Join(home, "state.db"), unmergedWT.ID); got != "active" { + t.Fatalf("unmerged worktree status = %q, want active", got) + } + if branches := runGitOutput(t, localPath, "branch", "--list", squashWT.Branch); strings.TrimSpace(branches) != "" { + t.Fatalf("squash branch still exists: %q", branches) + } + if branches := runGitOutput(t, localPath, "branch", "--list", unmergedWT.Branch); !strings.Contains(branches, unmergedWT.Branch) { + t.Fatalf("unmerged branch missing: %q", branches) + } +} + func TestWorktreeUnlockClearsStaleAndRefusesLiveLock(t *testing.T) { ctx := context.Background() home := filepath.Join(t.TempDir(), ".devstrap") @@ -167,6 +251,43 @@ func TestWorktreeUnlockClearsStaleAndRefusesLiveLock(t *testing.T) { } } +type testWorktree struct { + ID string `json:"id"` + Path string `json:"path"` + Branch string `json:"branch"` +} + +func listWorktreesForTest(t *testing.T, home, root string) []testWorktree { + t.Helper() + stdout, stderr, err := executeForTest("--home", home, "--root", root, "worktree", "list", "--json") + if err != nil { + t.Fatalf("worktree list stdout = %q stderr = %q err = %v", stdout, stderr, err) + } + var worktrees []testWorktree + if err := json.Unmarshal([]byte(stdout), &worktrees); err != nil { + t.Fatalf("decode worktree list: %v\n%s", err, stdout) + } + return worktrees +} + +func worktreeStatusForTest(t *testing.T, dbPath, wtID string) string { + t.Helper() + q := url.Values{} + q.Add("_pragma", "busy_timeout(5000)") + q.Add("_pragma", "foreign_keys(1)") + dsn := (&url.URL{Scheme: "file", Path: dbPath, RawQuery: q.Encode()}).String() + db, err := sql.Open("sqlite", dsn) + if err != nil { + t.Fatal(err) + } + defer func() { _ = db.Close() }() + var status string + if err := db.QueryRow("SELECT status FROM worktrees WHERE id = ?", wtID).Scan(&status); err != nil { + t.Fatal(err) + } + return status +} + func itoa(n int) string { return strconv.Itoa(n) } diff --git a/internal/git/git.go b/internal/git/git.go index d3aa81b..4203f67 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -503,6 +503,48 @@ func (r Runner) RevParse(ctx context.Context, dir, ref string) (string, error) { return r.Run(ctx, dir, "rev-parse", ref) } +// IsSquashMerged reports whether branch's content is already contained in the +// CURRENT baseRef tree — the content-equivalence test behind `worktree cleanup +// --merged`'s squash/rebase detection (P4-GIT-04). It simulates the merge +// (`git merge-tree --write-tree `, git >= 2.38): when the +// resulting tree is identical to baseRef's own tree, merging the branch would +// contribute nothing — every change the branch carries is already present in +// base, which is exactly the effect of a squash- or rebase-merge. Comparing +// against the CURRENT tree (rather than patch-id history) means a change that +// was merged and then REVERTED on base correctly reads as NOT merged +// (dual-review finding: historical patch-id equivalence would have deleted +// genuinely-unmerged work). +// +// Conservative by construction: a conflicting simulated merge (content +// diverged), an invalid ref, or an older git without --write-tree all report +// false — doubt is never grounds to reap. +// +// Documented accepted limitation (inherent to ANY content-equivalence test): +// a branch whose net change ALSO landed via an unrelated identical commit is +// indistinguishable from a squash-merge and reads as merged; the reap +// breadcrumb (the printed branch tip SHA) is the recovery path. Pinned by +// TestIsSquashMergedMatchesCoincidentallyIdenticalDiff. +func (r Runner) IsSquashMerged(ctx context.Context, dir, branch, baseRef string) (bool, error) { + if !safeBranchName(branch) { + return false, fmt.Errorf("invalid git branch name %q", branch) + } + if !safeBranchName(baseRef) { + return false, fmt.Errorf("invalid git base ref %q", baseRef) + } + merged, err := r.Run(ctx, dir, "merge-tree", "--write-tree", baseRef, branch) + if err != nil { + // Exit 1 is a conflicting simulated merge — the canonical not-merged + // answer. Any other failure (old git, bad ref) is equally + // conservative: never reap on doubt. + return false, nil + } + baseTree, err := r.Run(ctx, dir, "rev-parse", baseRef+"^{tree}") + if err != nil { + return false, err + } + return strings.TrimSpace(merged) == strings.TrimSpace(baseTree), nil +} + func (r Runner) WorktreeAdd(ctx context.Context, dir, path, branch, base string) error { if !safeBranchName(branch) { return fmt.Errorf("invalid git branch name %q", branch) diff --git a/internal/git/git_test.go b/internal/git/git_test.go index a181d32..ca4f3d0 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -176,6 +176,115 @@ exit 128 } } +func TestIsSquashMergedDetectsSquashMerge(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "one.txt", "one\n", "feature one") + writeAndCommit(t, runner.Bin, repo, "two.txt", "two\n", "feature two") + runRealGit(t, runner.Bin, repo, "checkout", "main") + runRealGit(t, runner.Bin, repo, "merge", "--squash", "feature") + runRealGit(t, runner.Bin, repo, "commit", "-m", "squash feature") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if !got { + t.Fatal("IsSquashMerged = false, want true") + } +} + +func TestIsSquashMergedDetectsRebaseMerge(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "one.txt", "one\n", "feature one") + writeAndCommit(t, runner.Bin, repo, "two.txt", "two\n", "feature two") + runRealGit(t, runner.Bin, repo, "checkout", "main") + writeAndCommit(t, runner.Bin, repo, "main.txt", "main\n", "advance main") + runRealGit(t, runner.Bin, repo, "cherry-pick", "feature~1", "feature") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if !got { + t.Fatal("IsSquashMerged = false, want true") + } +} + +func TestIsSquashMergedFalseForUnmerged(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "one.txt", "one\n", "feature one") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if got { + t.Fatal("IsSquashMerged = true, want false") + } +} + +func TestIsSquashMergedConservativeOnContentDivergence(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "README.md", "feature\n", "feature readme") + runRealGit(t, runner.Bin, repo, "checkout", "main") + writeAndCommit(t, runner.Bin, repo, "README.md", "main\n", "main readme") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if got { + t.Fatal("IsSquashMerged = true, want false") + } +} + +// TestIsSquashMergedFalseAfterRevertOnBase (dual-review fix): a change that +// was merged into base and then REVERTED must read as NOT merged — the branch +// still carries work absent from the CURRENT base tree. The earlier patch-id +// approach matched the historical (reverted) commit and would have reaped it. +func TestIsSquashMergedFalseAfterRevertOnBase(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "one.txt", "one\n", "feature one") + runRealGit(t, runner.Bin, repo, "checkout", "main") + runRealGit(t, runner.Bin, repo, "merge", "--squash", "feature") + runRealGit(t, runner.Bin, repo, "commit", "-m", "squash feature") + runRealGit(t, runner.Bin, repo, "revert", "--no-edit", "HEAD") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if got { + t.Fatal("IsSquashMerged = true, want false (squash was reverted on base)") + } +} + +// TestIsSquashMergedMatchesCoincidentallyIdenticalDiff pins the DOCUMENTED +// accepted limitation: a branch whose net change also landed via an unrelated +// identical commit is indistinguishable from a squash-merge by any content- +// equivalence test and reads as merged. If this test ever fails, the +// limitation was fixed — update spec/08 and the reap-breadcrumb rationale. +func TestIsSquashMergedMatchesCoincidentallyIdenticalDiff(t *testing.T) { + repo, runner := initSquashMergeRepo(t) + runRealGit(t, runner.Bin, repo, "checkout", "-b", "feature") + writeAndCommit(t, runner.Bin, repo, "one.txt", "identical\n", "feature adds one.txt") + runRealGit(t, runner.Bin, repo, "checkout", "main") + writeAndCommit(t, runner.Bin, repo, "one.txt", "identical\n", "unrelated identical change") + + got, err := runner.IsSquashMerged(context.Background(), repo, "feature", "main") + if err != nil { + t.Fatalf("IsSquashMerged: %v", err) + } + if !got { + t.Fatal("IsSquashMerged = false; the documented coincidental-identical-diff limitation appears fixed — update spec/08") + } +} + func TestCloneRetriesTransientNetworkErrors(t *testing.T) { countPath := filepath.Join(t.TempDir(), "count") script := writeFakeGit(t, fmt.Sprintf(`#!/bin/sh @@ -755,6 +864,33 @@ func initLocalDefaultBranchRepo(t *testing.T) (string, Runner) { return repo, Runner{Bin: gitBin, Timeout: 5 * time.Second} } +func initSquashMergeRepo(t *testing.T) (string, Runner) { + t.Helper() + gitBin, err := exec.LookPath("git") + if err != nil { + t.Skip("git not installed") + } + repo := filepath.Join(t.TempDir(), "repo") + if err := os.MkdirAll(repo, 0o755); err != nil { + t.Fatal(err) + } + runRealGit(t, gitBin, repo, "init") + runRealGit(t, gitBin, repo, "config", "user.name", "t") + runRealGit(t, gitBin, repo, "config", "user.email", "t@example.com") + runRealGit(t, gitBin, repo, "checkout", "-b", "main") + writeAndCommit(t, gitBin, repo, "README.md", "base\n", "init") + return repo, Runner{Bin: gitBin, Timeout: 5 * time.Second} +} + +func writeAndCommit(t *testing.T, gitBin, repo, name, contents, message string) { + t.Helper() + if err := os.WriteFile(filepath.Join(repo, name), []byte(contents), 0o644); err != nil { + t.Fatal(err) + } + runRealGit(t, gitBin, repo, "add", name) + runRealGit(t, gitBin, repo, "commit", "-m", message) +} + func runRealGit(t *testing.T, gitBin, dir string, args ...string) { t.Helper() cmd := exec.Command(gitBin, args...) diff --git a/spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md b/spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md index e0cf3ef..e51220b 100644 --- a/spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md +++ b/spec/08_GIT_MATERIALIZATION_AND_WORKTREES.md @@ -297,6 +297,10 @@ rebase: ask agent rerun: ask ``` +`devstrap worktree cleanup --merged` treats a worktree as merged when either the worktree branch is an ancestor of its recorded `base_ref` or the branch's patch content is already present on that base. Before each mergedness check it best-effort fetches the recorded base ref (for example `origin/main`) under the same project repo-lock pattern used by fresh worktree creation, deduped per (project, base ref) per run; if that fetch fails, cleanup warns and continues against the locally-known base ref. That offline fallback is deliberate, not an oversight: cleanup must work without network (the same reason forge cross-checks are a non-goal), the ancestry check has always evaluated against the local ref (the fetch is a freshness improvement this feature ADDED, not a guarantee it weakened), and merged-ness is by definition evaluated against the base as this machine knows it — a branch reaped offline is one whose content is already part of the locally-known base history, and the printed tip SHA restores it if the remote has since diverged. + +The content-equivalence path is intentionally offline and forge-agnostic: `git.Runner.IsSquashMerged` simulates the merge with `git merge-tree --write-tree ` (git >= 2.38) and reports merged only when the resulting tree is IDENTICAL to the current base tree — merging the branch would contribute nothing, which is exactly the effect of a squash-, rebase-, or cherry-pick-merge. Comparing against the CURRENT base tree (not patch-id history — the dual-review finding that killed the first draft) means a change that was merged and then reverted on base correctly reads as NOT merged. The conservative rule governs the content test itself: a conflicting simulated merge, any git error (including an older git without `--write-tree`), or unproven equivalence means "not merged", so the equivalence probe never affirms on doubt (staleness of the base is bounded separately, by the best-effort fetch above). One limitation is inherent to any content-equivalence test and accepted deliberately: a branch whose net change also landed via an unrelated, coincidentally identical commit is indistinguishable from a squash-merge and is reaped (pinned by `TestIsSquashMergedMatchesCoincidentallyIdenticalDiff`); as the recovery breadcrumb, every reap prints the deleted branch's tip SHA so `git branch ` restores it until git gc. Explicit forge cross-checks such as `gh pr list --state merged` are a non-goal for the materialization layer. + ## Duplicate clone detection During scan: diff --git a/spec/13_CLI_DAEMON_API.md b/spec/13_CLI_DAEMON_API.md index 74c3686..92ba39e 100644 --- a/spec/13_CLI_DAEMON_API.md +++ b/spec/13_CLI_DAEMON_API.md @@ -353,7 +353,7 @@ devstrap worktree cleanup --merged [--force] devstrap worktree unlock work/acme/api [--force] ``` -Current implementation requires `--fresh-upstream` for `worktree new`, fetches `origin/` before resolving the base SHA, writes a per-repo lock under `~/.devstrap/locks`, records worktree metadata, honors the stored LFS policy by either running `git lfs pull` or warning about pointer files, and refuses dirty worktree removal unless `--force` is explicit. `worktree remove --force` handles manually deleted worktree paths by running `git worktree prune` from the main checkout and marking the DB row removed. `worktree status ` re-fetches the recorded base ref and reports whether the worktree is fresh or stale. `worktree finalize ` reuses the same stale-base check and exits non-zero if the base moved unless `--allow-stale-base` is set. `cleanup --merged` removes clean, merged worktrees, prunes stale missing paths, reports a skipped count for unreadable or dirty worktrees, and only removes a merged-but-dirty worktree when `--force` is set. `worktree unlock ` reports the holder of a project's repo operation lock and clears it when the holder is dead/stale (or when `--force` is set), providing a recovery path after a crash; `doctor` also lists held locks. The default-branch resolution for `worktree new` confirms the remote default authoritatively via `git ls-remote --symref origin HEAD`, repairing a missing `origin/HEAD` with `git remote set-head origin --auto` and warning if the result is not authoritative. +Current implementation requires `--fresh-upstream` for `worktree new`, fetches `origin/` before resolving the base SHA, writes a per-repo lock under `~/.devstrap/locks`, records worktree metadata, honors the stored LFS policy by either running `git lfs pull` or warning about pointer files, and refuses dirty worktree removal unless `--force` is explicit. `worktree remove --force` handles manually deleted worktree paths by running `git worktree prune` from the main checkout and marking the DB row removed. `worktree status ` re-fetches the recorded base ref and reports whether the worktree is fresh or stale. `worktree finalize ` reuses the same stale-base check and exits non-zero if the base moved unless `--allow-stale-base` is set. `cleanup --merged` removes clean, merged worktrees, prunes stale missing paths, reports a skipped count for unreadable or dirty worktrees, and only removes a merged-but-dirty worktree when `--force` is set. Merged-ness is ancestry OR content-equivalence (`P4-GIT-04`): the `git branch --merged` ancestry check runs first, and its misses consult `git.Runner.IsSquashMerged` — a current-tree merge probe (`git merge-tree --write-tree `, git ≥ 2.38) that reports merged only when the simulated merge's tree is identical to the current base tree, catching GitHub "Squash and merge", rebase-, and cherry-pick-merges while correctly treating a merged-then-reverted change as NOT merged — with the conservative rule that a conflicting merge, an older git, or any error means NOT merged (a false positive would delete a real worktree and its branch). Reaps are labeled `merged` vs `merged (squash)`, the recorded `remote/branch` base is best-effort fetched under the repo lock first (warn + continue offline), reaped worktrees also get `git branch -D` (warn-only on failure), and forge-API (`gh pr list`) cross-checks are an explicit non-goal so cleanup works offline. `worktree unlock ` reports the holder of a project's repo operation lock and clears it when the holder is dead/stale (or when `--force` is set), providing a recovery path after a crash; `doctor` also lists held locks. The default-branch resolution for `worktree new` confirms the remote default authoritatively via `git ls-remote --symref origin HEAD`, repairing a missing `origin/HEAD` with `git remote set-head origin --auto` and warning if the result is not authoritative. ## Agent commands diff --git a/spec/16_TEST_PLAN.md b/spec/16_TEST_PLAN.md index 1024b9d..fd9f49c 100644 --- a/spec/16_TEST_PLAN.md +++ b/spec/16_TEST_PLAN.md @@ -43,7 +43,7 @@ The Phase 0 suite must cover: - `sync --hub-file --dry-run` exposes the file-backed hub plan without writing; - `add -> hydrate` against a local bare remote, refusal to write skeletons into non-empty directories, missing-remote clone failure preserving the original skeleton without temp-dir leaks, and promotion-time dirty-target refusal without removing local files; - repo operation locks reject active concurrent operations and reclaim stale same-host owners before hydrate/worktree mutation; -- fresh worktree creation from an advanced remote SHA while local clone state is stale, collision-resistant worktree branch naming with retry, `worktree status` reporting stale after the remote base advances again, `worktree finalize` refusing stale bases unless `--allow-stale-base`, LFS-policy warning/pull branching for agent worktrees, and cleanup of DB-invisible checkouts plus `agent/...` branches when post-add LFS or SQLite insert failures occur; +- fresh worktree creation from an advanced remote SHA while local clone state is stale, collision-resistant worktree branch naming with retry, `worktree status` reporting stale after the remote base advances again, `worktree finalize` refusing stale bases unless `--allow-stale-base`, LFS-policy warning/pull branching for agent worktrees, `IsSquashMerged` merge-tree content equivalence for squash-merge and rebase/cherry-pick with conservative false cases (unmerged, content divergence, revert-on-base) plus the pinned coincidental-identical-diff limitation, `worktree cleanup --merged` reaping a squash-merged clean worktree while leaving an unmerged worktree active, and cleanup of DB-invisible checkouts plus `agent/...` branches when post-add LFS or SQLite insert failures occur; - forge detection/routing for `agent pr` across GitHub/GitLab/Gitea/Forgejo/Bitbucket/Azure-style remotes, forge-specific token env allowlists, Azure remote-key folding, hermetic SSH host-alias resolution tests using a PATH-shimmed `ssh -G`, and graceful unknown-forge compare-URL fallback; - HLC monotonic send/receive, max-skew rejection, logical-counter overflow behavior, persisted local event HLC/sequence stamping and previous-hash linking across reopen, transactional idempotent event apply, divergent duplicate event quarantine, incoming `prev_event_hash` chain-break rejection with conflict recording, per-event `event_verification_failure` quarantine (a revoked device's event mid-batch: valid neighbors still apply, the safe cursor advances past it, exactly one deduped conflict row survives repeated pulls; `errors.Is` sentinel coverage for every `verifyEventSignature` failure path while infrastructure errors stay non-matching; approve-time replay applies the quarantined event and resolves its conflict; e2e `sync_revoked_quarantine.txtar` proves a revoked device's push cannot wedge other devices — `P6-SYNC-01`), grant-carrier verification before WCK ingestion (`EncryptedHub.Pull` refuses/ingests/nil-verifier back-compat; `VerifyRemoteEvent` matches the `insertEvent` trust regime across {local, approved+valid, forged sig, revoked, unknown} × {enrolled, not}; and the malicious-hub acceptance test `TestSyncRejectsForgedGrantBeforeWCKIngest` — a forged grant wrapping an attacker WCK to the victim's own recipient at epoch 2^40 leaves `CurrentKeyEpoch`, the key store, and the keyring untouched and lands as one quarantine conflict — `P6-SEC-01`), founder/join workspace-key bootstrap (`init` writes `role: founder`/`joiner` and mints no key; `init --join` prints approval-first next steps; e2e `sync_join_flow.txtar` proves a `--join` device that adds a project and syncs BEFORE approval defers its push — `pushed 0`, `Awaiting workspace key grant`, nothing leaked to the hub — and after approval its pre-approval project pushes and materializes on the founder, hub ciphertext throughout — `P6-SEC-02`), HLC-gated delete tombstone restore/ignore behavior, order-independent same-path/different-remote conflict protection with stable conflict details, and origin-atomic draft-snapshot recording (`TestInsertLocalEventTxMatchesInsertLocalEvent` pins `InsertLocalEventTx` parity with `InsertLocalEvent`; `TestDraftSnapshotCreateRecordsOriginSnapshotRow` and `TestRewrapDraftBlobRecordsOriginSupersedingSnapshot` prove `draft snapshot create` and the revoke rewrap write the origin's `draft_snapshots` row in one transaction with the event; `TestApplyDraftSnapshotUnknownProjectQuarantinesWithoutAbort`, `TestApplyDraftSnapshotTombstonedProjectDrops`, `TestApplyDraftSnapshotMalformedPayloadQuarantinesWithoutAbort`, and `TestReplayPendingDraftSnapshotConflictRecovers` pin the draft pending-project quarantine/replay path; e2e `draft_snapshot_gc_retains_origin.txtar` proves the origin's bundle blob survives `sync` + `hub gc` — `P6-DATA-01`), and sticky fail-closed enrollment (`TestHasEnrolledDevicesStickyAfterRevoke` pins the predicate — pending placeholders don't close the window, approved does, revoked/lost keep it closed; `TestApplyEventsRevokedLastDeviceStaysFailClosed` proves that with only a revoked device on record a validly-signed event from it, an unknown-device event, a signed pending-device event, and an unsigned no-key-device event all quarantine instead of applying — `P6-SYNC-03`). diff --git a/spec/18_WORK_LOG.md b/spec/18_WORK_LOG.md index 068028b..240e1ba 100644 --- a/spec/18_WORK_LOG.md +++ b/spec/18_WORK_LOG.md @@ -31,6 +31,20 @@ Follow-ups: Entries are newest-first: each code-modifying cycle prepends ONE dated entry at the top. +## 2026-07-06 — feat(worktree): cleanup reaps squash/rebase-merged worktrees (P4-GIT-04) + +Changed: +- `internal/git/git.go`: new `Runner.IsSquashMerged(ctx, dir, branch, baseRef)` — offline content-equivalence via a **current-tree merge probe**: `git merge-tree --write-tree ` (git ≥ 2.38) reports merged only when the simulated merge's tree is IDENTICAL to the current base tree (the branch would contribute nothing — exactly a squash/rebase/cherry-pick-merge's effect). The first draft's `git cherry`/synthetic-commit/`patch-id --stable` chain was REPLACED in dual review: both reviewers independently proved (with repros) that patch-id equivalence matches HISTORICAL base commits, so a change merged-then-REVERTED on base — or a per-commit coincidence overruling cherry — would reap genuinely-unmerged work; the merge-tree probe compares against the current tree and is immune to the revert class. Conservative: a conflicting simulated merge, an older git, or any error → not merged. +- Documented accepted limitation (inherent to ANY content test): a branch whose net change also landed via an unrelated identical commit reads as merged — pinned by `TestIsSquashMergedMatchesCoincidentallyIdenticalDiff` (a tripwire naming spec/08), mitigated by the reap breadcrumb below. +- `internal/cli/worktree.go`: `worktree cleanup --merged` semantics extend to ancestry OR content-equivalent — the ancestry `git branch --merged` check stays first; only its misses consult `IsSquashMerged`. Reaps are labeled `merged` vs `merged (squash)` and print the deleted branch's tip SHA (`branch was at `) as the recovery breadcrumb (`git branch ` restores until gc). New `refreshWorktreeBase` best-effort fetches the recorded `remote/branch` base under the repo lock, deduped per (project, base ref) per run (review finding: N worktrees must not trigger N fetches); warn + continue on stale/offline. Reaped worktrees also get `git branch -D` (warn-only), matching the P6-GIT-05 failure-cleanup precedent. +- Specs: spec/08 (merge-tree semantics, conservative rule, pinned limitation + breadcrumb, forge-API non-goal), spec/16 (test inventory). + +Validated: +- New real-git tests: `TestIsSquashMergedDetectsSquashMerge`, `...DetectsRebaseMerge`, `...FalseForUnmerged`, `...ConservativeOnContentDivergence`, `...FalseAfterRevertOnBase` (the dual-review repro), `...MatchesCoincidentallyIdenticalDiff` (documented-limitation tripwire); CLI e2e `TestWorktreeCleanupReapsSquashMergedWorktree` (squash-merged reaped with `merged (squash)` + branch deleted + state row `removed`; unmerged untouched in the same run). +- gofmt clean; `go test -race ./...` all ok; golangci-lint; spec-drift vs origin/main. + +Follow-ups: +- None (P4-GIT-04 moves to Recently shipped in the ledger). ## 2026-07-06 — feat(devices): self-healing WCK rotation after revoke (#134) Changed: