From 2567bfacbfdc50483b37e0e69db49741a400df94 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 16:07:47 -0700 Subject: [PATCH 01/18] fix(hooks): register Entire with Lefthook instead of owning its hook files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lefthook regenerates every file in .git/hooks from its own config, gated by a checksum of that config, and it does so at the top of whatever hook fires next — typically the pre-commit of the very commit being made. Entire's turn-start self-heal therefore always loses the race: any `lefthook install -f` (npm postinstall) or lefthook.yml edit silently takes the hook back, and the next commit is not captured. #2264 is the symptom users see — `entire status` naming a checkpoint destination while no hook existed to reach it. Stop wanting the file. Entire writes a config of its own (entire-lefthook.yml) declaring a script per hook, registers it with a single `extends` entry in Lefthook's local config, and lets Lefthook invoke those scripts. Lefthook resolves `extends` at run time, so this takes effect with no `lefthook install` and no user action, and Lefthook's own config is never touched. Hook-file ownership follows from that: once Lefthook dispatches Entire, Entire owning the file too would run every hook twice, so the native install skips any hook carrying Lefthook's launcher and reconcileHookFiles moves a launcher Entire had displaced back over its own hook. It also clears the .old backups that otherwise make every later Lefthook sync fail with "can't rename pre-push to pre-push.old" (#1349). A hook Lefthook has not taken over is still installed natively: in a repo where nobody ran `lefthook install`, Entire's own hooks are the only delivery there is. CheckHookDelivery is now the single question `entire status` and `entire doctor` ask, and in a Lefthook repo it is answered by the registration rather than by .git/hooks/*. Status gained a delivery line plus hooks_deliver / hooks_manager / hooks_deliver_reason in --json; `doctor --force` re-registers and reconciles. detectHookManagers' advice was wrong in both directions and is corrected: pre-commit 4.6.2, Overcommit 0.73.0 and hk 1.58.1 all overwrite Entire's hooks at install (verified), and Lefthook no longer needs any user action at all. Verified end-to-end against lefthook 2.1.10: hooks dispatch exactly once per commit, the user's own Lefthook commands keep running, the three artifacts are ignored via .git/info/exclude, and uninstall removes only what Entire wrote. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29BM8KGTDZXD1N3PJ3HGQFW --- cmd/entire/cli/doctor.go | 33 + cmd/entire/cli/setup.go | 9 + cmd/entire/cli/status.go | 32 + cmd/entire/cli/status_test.go | 50 ++ cmd/entire/cli/strategy/common.go | 39 + cmd/entire/cli/strategy/hook_managers.go | 68 +- cmd/entire/cli/strategy/hook_managers_test.go | 65 +- cmd/entire/cli/strategy/hooks.go | 19 +- cmd/entire/cli/strategy/lefthook.go | 701 ++++++++++++++++++ cmd/entire/cli/strategy/lefthook_test.go | 428 +++++++++++ 10 files changed, 1384 insertions(+), 60 deletions(-) create mode 100644 cmd/entire/cli/strategy/lefthook.go create mode 100644 cmd/entire/cli/strategy/lefthook_test.go diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index bbbf009690..b4dfd1b59c 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -632,6 +632,39 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { ctx := cmd.Context() w := cmd.OutOrStdout() + // A Lefthook repo is judged on Entire's registration with Lefthook, not on + // .git/hooks/*: Lefthook owns those files and rewrites them constantly, so + // their contents say nothing about whether Entire runs. Every other + // manager only overwrites at install time, so for those the hook files + // remain the answer and the check below is the right one. + if delivery := strategy.CheckHookDelivery(ctx, false); delivery.Manager == strategy.LefthookManagerName { + if delivery.OK { + // --force also reconciles .git/hooks/*, which is where a repo left + // mid-fight — Entire's own hook chaining to Lefthook's displaced + // launcher, so every hook runs Entire twice — gets straightened + // out. Delivery reads OK in that state, so it needs asking for. + if force { + if _, err := strategy.EnsureLefthookIntegration(ctx, false); err != nil { + return fmt.Errorf("reconcile %s hooks: %w", delivery.Manager, err) + } + } + fmt.Fprintf(w, "✓ Git hooks: OK (via %s)\n", delivery.Manager) + return nil + } + fmt.Fprintf(w, "Git hooks: NOT DELIVERING (%s)\n", delivery.Manager) + fmt.Fprintf(w, " %s\n", delivery.Reason) + fmt.Fprintln(w, " Fix: re-register Entire in Lefthook's configuration.") + if !force { + fmt.Fprintln(w, " Run `entire doctor --force` to apply it.") + return nil + } + if _, err := strategy.EnsureLefthookIntegration(ctx, false); err != nil { + return fmt.Errorf("register Entire with Lefthook: %w", err) + } + fmt.Fprintln(w, " ✓ Re-registered") + return nil + } + switch strategy.CheckGitHookState(ctx) { case strategy.GitHooksCurrent: fmt.Fprintln(w, "✓ Git hooks: OK") diff --git a/cmd/entire/cli/setup.go b/cmd/entire/cli/setup.go index ae8cbe5f9a..65e7e42a13 100644 --- a/cmd/entire/cli/setup.go +++ b/cmd/entire/cli/setup.go @@ -2808,6 +2808,15 @@ func uninstallGitHooks(ctx context.Context, p *uninstallPrinter) bool { p.warnUnder("failed to remove git hooks: %v", err) return false } + // The Lefthook registration is separate from the hook files, so uninstall + // has to take both. Only artifacts carrying Entire's marker are removed. + lefthookRemoved, err := strategy.RemoveLefthookIntegration(ctx) + if err != nil { + p.stepFailed("Failed to remove the Lefthook integration") + p.warnUnder("failed to remove the Lefthook integration: %v", err) + return false + } + removed += lefthookRemoved if removed > 0 { p.step("Removed git hooks (%d)", removed) } else { diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index 4f6b3bed6b..e06fc89a25 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -247,6 +247,7 @@ func formatSettingsStatusShort(ctx context.Context, s *EntireSettings, sty statu // Where checkpoint data syncs (the single elected remote), and how many // checkpoints have not reached it yet. Local-only computation. if s.Enabled { + writeHookDeliveryLine(ctx, &b, s, sty) writeCheckpointSyncLines(ctx, &b, s, sty) } @@ -562,6 +563,26 @@ func countUnpushedCheckpointsForStatus(ctx context.Context, remoteName string) i return n } +// writeHookDeliveryLine reports whether Entire's hooks will fire. +// +// Without it status named a checkpoint destination while no hook existed to +// reach it, which is issue #2264. The destination line below is still shown — +// it is true, and status is the only place it appears — but this line above it +// removes the claim that delivery is working. +func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, s *EntireSettings, sty statusStyles) { + delivery := strategy.CheckHookDelivery(ctx, s.AbsoluteGitHookPath) + b.WriteString("\n") + switch { + case delivery.OK && delivery.Manager != "": + b.WriteString(sty.render(sty.dim, " Git hooks · via "+delivery.Manager)) + case delivery.OK: + b.WriteString(sty.render(sty.dim, " Git hooks · installed")) + default: + b.WriteString(sty.render(sty.yellow, " ! Checkpoints are NOT being captured: "+delivery.Reason)) + b.WriteString(sty.render(sty.dim, " · run 'entire doctor'")) + } +} + // writeCheckpointSyncLines reports the checkpoint sync destination (and the // unpushed counter, when non-zero) in the enabled status block, prefixed by the // disabled-pushing line when automatic pushing is off. No remotes configured @@ -1057,6 +1078,13 @@ type statusJSON struct { CheckpointSyncRemote string `json:"checkpoint_sync_remote,omitempty"` CheckpointSyncRemoteSource string `json:"checkpoint_sync_remote_source,omitempty"` // config|observed|default|sole|first|dedicated CheckpointSyncError string `json:"checkpoint_sync_error,omitempty"` // fail-closed message + // HooksDeliver reports whether Entire's Git hooks will actually fire, and + // HooksManager names the hook manager that owns the files when one does. + // Without these, status could report a destination while nothing would + // reach it (#2264). + HooksDeliver bool `json:"hooks_deliver"` + HooksManager string `json:"hooks_manager,omitempty"` + HooksDeliverReason string `json:"hooks_deliver_reason,omitempty"` // CheckpointReadSourceUnknown reports that the read-source probe failed, // so no read source could be determined. Emitted only alongside // checkpoint_push_disabled, and checkpoint_sync_remote is then absent @@ -1171,6 +1199,10 @@ func runStatusJSON(ctx context.Context, w io.Writer) error { result.CheckpointSyncRemote = syncInfo.Remote result.CheckpointSyncRemoteSource = syncInfo.Source result.CheckpointSyncError = syncInfo.Err + delivery := strategy.CheckHookDelivery(ctx, s.AbsoluteGitHookPath) + result.HooksDeliver = delivery.OK + result.HooksManager = delivery.Manager + result.HooksDeliverReason = delivery.Reason result.CheckpointReadFallback = syncInfo.ReadFallback result.CheckpointReadSourceUnknown = syncInfo.ReadSourceUnknown result.UnpushedCheckpoints = syncInfo.Unpushed diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 644708b81b..66a4df9724 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -23,6 +23,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/session" "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/strategy" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/entireio/cli/redact" @@ -3288,3 +3289,52 @@ func TestRunStatusDetailed_ReportsRejectedExternalAgents(t *testing.T) { //nolin t.Errorf("status does not name where the setting must live:\n%s", got) } } + +// #2264: status named a checkpoint destination while no hook existed to reach +// it. The delivery line is the answer, and in a Lefthook repo it must be +// judged on Entire's registration rather than on .git/hooks/*, which Lefthook +// owns and rewrites. +func TestRunStatus_HookDeliveryLine(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, testSettingsEnabled) + if err := os.WriteFile("lefthook.yml", []byte("pre-commit: {}\n"), 0o644); err != nil { + t.Fatalf("write lefthook.yml: %v", err) + } + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + if out := stdout.String(); !strings.Contains(out, "Checkpoints are NOT being captured") { + t.Errorf("unregistered Lefthook repo must warn, got:\n%s", out) + } + + if _, err := strategy.EnsureLefthookIntegration(context.Background(), false); err != nil { + t.Fatalf("EnsureLefthookIntegration() error = %v", err) + } + + stdout.Reset() + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + out := stdout.String() + if !strings.Contains(out, "Git hooks · via Lefthook") { + t.Errorf("registered Lefthook repo must report delivery, got:\n%s", out) + } + if strings.Contains(out, "NOT being captured") { + t.Errorf("registered Lefthook repo must not warn, got:\n%s", out) + } + + var jsonOut bytes.Buffer + if err := runStatusJSON(context.Background(), &jsonOut); err != nil { + t.Fatalf("runStatusJSON() error = %v", err) + } + var got statusJSON + if err := json.Unmarshal(jsonOut.Bytes(), &got); err != nil { + t.Fatalf("unmarshal status JSON: %v", err) + } + if !got.HooksDeliver || got.HooksManager != "Lefthook" || got.HooksDeliverReason != "" { + t.Errorf("hooks fields = %+v, want delivering via Lefthook: %s", got, jsonOut.String()) + } +} diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index df708383a3..4b60519704 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -111,6 +111,15 @@ func EnsureSetup(ctx context.Context) error { return fmt.Errorf("failed to ensure primary metadata ref: %w", err) } + // Lefthook first, because whether Lefthook runs Entire from its own config + // decides which hook files Entire should own — see installSkipsHook. Owning + // .git/hooks/* is no fix in a Lefthook repo anyway: Lefthook takes those + // files back at the top of its next run, which is typically the pre-commit + // of the very commit being made. + if err := ensureLefthookIntegrationIfManaged(ctx); err != nil { + return err + } + // Install generic hooks (they delegate to strategy at runtime) if !IsGitHookInstalled(ctx) { if _, err := ReinstallGitHooks(ctx); err != nil { @@ -1808,3 +1817,33 @@ func prepareTranscriptIfNeeded(ctx context.Context, ag agent.Agent, transcriptPa _ = preparer.PrepareTranscript(ctx, transcriptPath) //nolint:errcheck // Best-effort in hook path } } + +// ensureLefthookIntegrationIfManaged installs or repairs Entire's Lefthook +// registration when this repository is Lefthook-managed. A repo that declines +// the integration (a non-YAML local config Entire will not shadow) keeps the +// native hooks and is not an error. +// +// An already-current integration still reconciles the hook files, because +// that is where a repo left mid-fight — Entire's hook chaining to Lefthook's +// displaced launcher, running Entire twice — gets straightened out. +func ensureLefthookIntegrationIfManaged(ctx context.Context) error { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil || !LefthookManaged(repoRoot) { + return nil //nolint:nilerr // not a Lefthook repo: nothing to do + } + absolute := hookSettingsFromConfig(ctx) + current, err := LefthookIntegrationCurrent(ctx, absolute) + if err != nil { + return nil //nolint:nilerr // inspection failure falls back to native hooks + } + if current { + return reconcileHookFiles(ctx, repoRoot) + } + if _, err := EnsureLefthookIntegration(ctx, absolute); err != nil { + if errors.Is(err, ErrLefthookLocalConfigUnwritable) { + return nil + } + return fmt.Errorf("failed to register Entire with Lefthook: %w", err) + } + return nil +} diff --git a/cmd/entire/cli/strategy/hook_managers.go b/cmd/entire/cli/strategy/hook_managers.go index 9e7134548e..8f8849c3f6 100644 --- a/cmd/entire/cli/strategy/hook_managers.go +++ b/cmd/entire/cli/strategy/hook_managers.go @@ -13,28 +13,33 @@ import ( // hookManager describes an external hook manager detected in a repository. type hookManager struct { - Name string // "Husky", "Lefthook", "pre-commit", "Overcommit" - ConfigPath string // relative path that triggered detection (e.g., ".husky/") - OverwritesHooks bool // true if the tool will overwrite Entire's hooks on reinstall + Name string // "Husky", "Lefthook", "pre-commit", "Overcommit", "hk" + ConfigPath string // relative path that triggered detection (e.g., ".husky/") } // detectHookManagers checks the repository root for known hook manager config // files/directories. Detection is filesystem-only (os.Stat, no file reads). +// +// Every manager listed here overwrites Entire's hooks when it installs them — +// verified against pre-commit 4.6.2 ("Use -f to use only pre-commit", which +// saves a .pre-commit.legacy copy), Overcommit 0.73.0 ("Moving old hooks"), and +// hk 1.58.1. What makes Lefthook different, and what EnsureLefthookIntegration +// exists for, is that it also reclaims .git/hooks/* at the top of every later +// run, so reinstalling on the next agent turn never wins the race. func detectHookManagers(repoRoot string) []hookManager { var managers []hookManager checks := []hookManager{ - {"Husky", ".husky/", true}, - {"pre-commit", ".pre-commit-config.yaml", false}, - {"Overcommit", ".overcommit.yml", false}, + {"Husky", ".husky/"}, + {"pre-commit", ".pre-commit-config.yaml"}, + {"Overcommit", ".overcommit.yml"}, } // Lefthook supports {.,}lefthook{,-local}.{yml,yaml,json,toml} for _, prefix := range []string{"", "."} { for _, variant := range []string{"", "-local"} { for _, ext := range []string{"yml", "yaml", "json", "toml"} { - name := prefix + "lefthook" + variant + "." + ext - checks = append(checks, hookManager{"Lefthook", name, false}) + checks = append(checks, hookManager{LefthookManagerName, prefix + "lefthook" + variant + "." + ext}) } } } @@ -42,8 +47,7 @@ func detectHookManagers(repoRoot string) []hookManager { // hk supports {.config/,}hk{,.local}.pkl for _, dir := range []string{"", ".config/"} { for _, variant := range []string{"", ".local"} { - name := dir + "hk" + variant + ".pkl" - checks = append(checks, hookManager{"hk", name, false}) + checks = append(checks, hookManager{"hk", dir + "hk" + variant + ".pkl"}) } } @@ -71,34 +75,36 @@ func hookManagerWarning(managers []hookManager, cmdPrefix string) string { var b strings.Builder - specs := buildHookSpecs(cmdPrefix) - for _, m := range managers { - if m.OverwritesHooks { - fmt.Fprintf(&b, "Warning: %s detected (%s)\n", m.Name, m.ConfigPath) - fmt.Fprintf(&b, "\n") + switch m.Name { + case LefthookManagerName: + // Entire registers itself in Lefthook's own config, so Lefthook's + // regenerations no longer remove it and there is nothing for the + // user to do. See EnsureLefthookIntegration. + fmt.Fprintf(&b, "Note: %s detected (%s)\n\n", m.Name, m.ConfigPath) + fmt.Fprintf(&b, " %s regenerates Git hooks whenever its config changes or it runs.\n", m.Name) + fmt.Fprintf(&b, " Entire registers itself in %s's own configuration instead of owning\n", m.Name) + fmt.Fprintf(&b, " the hook files, so those regenerations no longer remove it.\n") + fmt.Fprintf(&b, " No action needed.\n\n") + case "Husky": + // Husky's hooks are hand-editable shell scripts it does not + // rewrite, so the durable fix is to add Entire's line to them. + fmt.Fprintf(&b, "Warning: %s detected (%s)\n\n", m.Name, m.ConfigPath) fmt.Fprintf(&b, " %s may overwrite hooks installed by Entire on npm install.\n", m.Name) - fmt.Fprintf(&b, " To make Entire hooks permanent, add these lines to your %s hook files:\n", m.Name) - fmt.Fprintf(&b, "\n") - - // Use the config path as the hook directory prefix for hook files. - // For Husky, this is typically ".husky/" where hook scripts are stored. - hookDir := m.ConfigPath - - for _, spec := range specs { + fmt.Fprintf(&b, " To make Entire hooks permanent, add these lines to your %s hook files:\n\n", m.Name) + for _, spec := range buildHookSpecs(cmdPrefix) { cmdLine := extractCommandLine(spec.content) if cmdLine == "" { continue } - fmt.Fprintf(&b, " %s%s:\n", hookDir, spec.name) - fmt.Fprintf(&b, " %s\n", cmdLine) - fmt.Fprintf(&b, "\n") + fmt.Fprintf(&b, " %s%s:\n", m.ConfigPath, spec.name) + fmt.Fprintf(&b, " %s\n\n", cmdLine) } - } else { - fmt.Fprintf(&b, "Note: %s detected (%s)\n", m.Name, m.ConfigPath) - fmt.Fprintf(&b, "\n") - fmt.Fprintf(&b, " If %s reinstalls hooks, run 'entire enable' to restore Entire's hooks.\n", m.Name) - fmt.Fprintf(&b, "\n") + default: + fmt.Fprintf(&b, "Warning: %s detected (%s)\n\n", m.Name, m.ConfigPath) + fmt.Fprintf(&b, " %s overwrites Entire's hooks when it installs its own.\n", m.Name) + fmt.Fprintf(&b, " Entire reinstalls them on the next agent turn, but a commit made in\n") + fmt.Fprintf(&b, " between is not captured. Run 'entire enable' to restore them now.\n\n") } } diff --git a/cmd/entire/cli/strategy/hook_managers_test.go b/cmd/entire/cli/strategy/hook_managers_test.go index d359eb8ded..9391a9c31a 100644 --- a/cmd/entire/cli/strategy/hook_managers_test.go +++ b/cmd/entire/cli/strategy/hook_managers_test.go @@ -38,9 +38,6 @@ func TestDetectHookManagers_Husky(t *testing.T) { if managers[0].ConfigPath != ".husky/" { t.Errorf("expected .husky/, got %s", managers[0].ConfigPath) } - if !managers[0].OverwritesHooks { - t.Error("Husky should have OverwritesHooks=true") - } } func TestDetectHookManagers_Lefthook(t *testing.T) { @@ -61,9 +58,6 @@ func TestDetectHookManagers_Lefthook(t *testing.T) { if managers[0].ConfigPath != "lefthook.yml" { t.Errorf("expected lefthook.yml, got %s", managers[0].ConfigPath) } - if managers[0].OverwritesHooks { - t.Error("Lefthook should have OverwritesHooks=false") - } } func TestDetectHookManagers_LefthookDotPrefix(t *testing.T) { @@ -165,9 +159,6 @@ func TestDetectHookManagers_PreCommit(t *testing.T) { if managers[0].ConfigPath != ".pre-commit-config.yaml" { t.Errorf("expected .pre-commit-config.yaml, got %s", managers[0].ConfigPath) } - if managers[0].OverwritesHooks { - t.Error("pre-commit should have OverwritesHooks=false") - } } func TestDetectHookManagers_Overcommit(t *testing.T) { @@ -188,9 +179,6 @@ func TestDetectHookManagers_Overcommit(t *testing.T) { if managers[0].ConfigPath != ".overcommit.yml" { t.Errorf("expected .overcommit.yml, got %s", managers[0].ConfigPath) } - if managers[0].OverwritesHooks { - t.Error("Overcommit should have OverwritesHooks=false") - } } func TestDetectHookManagers_Hk(t *testing.T) { @@ -211,9 +199,6 @@ func TestDetectHookManagers_Hk(t *testing.T) { if managers[0].ConfigPath != "hk.pkl" { t.Errorf("expected hk.pkl, got %s", managers[0].ConfigPath) } - if managers[0].OverwritesHooks { - t.Error("hk should have OverwritesHooks=false") - } } func TestDetectHookManagers_HkConfigDir(t *testing.T) { @@ -314,7 +299,7 @@ func TestHookManagerWarning_Husky(t *testing.T) { t.Parallel() managers := []hookManager{ - {Name: "Husky", ConfigPath: ".husky/", OverwritesHooks: true}, + {Name: "Husky", ConfigPath: ".husky/"}, } warning := hookManagerWarning(managers, "entire") @@ -348,26 +333,52 @@ func TestHookManagerWarning_Husky(t *testing.T) { } } -func TestHookManagerWarning_GitHooksManager(t *testing.T) { +func TestHookManagerWarning_Lefthook(t *testing.T) { t.Parallel() managers := []hookManager{ - {Name: "Lefthook", ConfigPath: "lefthook.yml", OverwritesHooks: false}, + {Name: "Lefthook", ConfigPath: "lefthook.yml"}, } warning := hookManagerWarning(managers, "entire") - // Category B: should be a Note, not a Warning + // Lefthook is the one manager Entire integrates with at the config level, + // so the message must not ask the user to do anything. if !strings.Contains(warning, "Note: Lefthook detected") { t.Error("warning should contain 'Note: Lefthook detected'") } - if !strings.Contains(warning, "run 'entire enable' to restore") { - t.Error("warning should mention running 'entire enable'") + if !strings.Contains(warning, "No action needed") { + t.Error("warning should say no action is needed") + } + if strings.Contains(warning, "entire enable") { + t.Error("warning should not tell the user to re-run entire enable") } - - // Should NOT contain hook file copy-paste instructions if strings.Contains(warning, "prepare-commit-msg:") { - t.Error("category B warning should not contain hook file instructions") + t.Error("warning should not contain hook file instructions") + } +} + +// pre-commit, Overcommit and hk each overwrite Entire's hooks at install time +// but do not reclaim them afterwards, so the next agent turn repairs it. +func TestHookManagerWarning_OverwritesAtInstall(t *testing.T) { + t.Parallel() + + for _, name := range []string{"pre-commit", "Overcommit", "hk"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + + warning := hookManagerWarning([]hookManager{{Name: name, ConfigPath: "cfg"}}, "entire") + + if !strings.Contains(warning, "Warning: "+name+" detected") { + t.Errorf("warning should name %s, got %q", name, warning) + } + if !strings.Contains(warning, "next agent turn") { + t.Error("warning should say Entire reinstalls on the next agent turn") + } + if !strings.Contains(warning, "entire enable") { + t.Error("warning should offer 'entire enable' to restore now") + } + }) } } @@ -389,7 +400,7 @@ func TestHookManagerWarning_AbsolutePathPrefix(t *testing.T) { t.Parallel() managers := []hookManager{ - {Name: "Husky", ConfigPath: ".husky/", OverwritesHooks: true}, + {Name: "Husky", ConfigPath: ".husky/"}, } // The prefix is whatever hookCmdPrefix resolved to — bare "entire" or, with @@ -408,8 +419,8 @@ func TestHookManagerWarning_Multiple(t *testing.T) { t.Parallel() managers := []hookManager{ - {Name: "Husky", ConfigPath: ".husky/", OverwritesHooks: true}, - {Name: "Lefthook", ConfigPath: "lefthook.yml", OverwritesHooks: false}, + {Name: "Husky", ConfigPath: ".husky/"}, + {Name: "Lefthook", ConfigPath: "lefthook.yml"}, } warning := hookManagerWarning(managers, "entire") diff --git a/cmd/entire/cli/strategy/hooks.go b/cmd/entire/cli/strategy/hooks.go index 6f6528b281..0b3cf75997 100644 --- a/cmd/entire/cli/strategy/hooks.go +++ b/cmd/entire/cli/strategy/hooks.go @@ -713,8 +713,17 @@ func InstallGitHook(ctx context.Context, silent, absolutePath bool) (int, error) } specs := buildHookSpecs(cmdPrefix) installedCount := 0 + // In a Lefthook repo the hooks Lefthook owns already run Entire from its + // own config; see installSkipsHook. + lefthookDelivers := lefthookDeliversHooks(ctx, absolutePath) + + skipped := 0 for _, spec := range specs { + if installSkipsHook(root, spec.name, lefthookDelivers) { + skipped++ + continue + } backupName := spec.name + backupSuffix backupExists := hookFileExists(root, backupName) @@ -758,8 +767,14 @@ func InstallGitHook(ctx context.Context, silent, absolutePath bool) (int, error) } if !silent { - fmt.Println("✓ Installed git hooks (prepare-commit-msg, commit-msg, post-commit, pre-push)") - fmt.Println(" Hooks delegate to the current strategy at runtime") + // Claiming an install Entire deliberately skipped is the same kind of + // false report this integration exists to stop. + if skipped == len(specs) { + fmt.Printf("✓ Git hooks run through %s (Entire is registered in its config)\n", LefthookManagerName) + } else { + fmt.Println("✓ Installed git hooks (prepare-commit-msg, commit-msg, post-commit, pre-push)") + fmt.Println(" Hooks delegate to the current strategy at runtime") + } } return installedCount, nil diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go new file mode 100644 index 0000000000..26ef525d7b --- /dev/null +++ b/cmd/entire/cli/strategy/lefthook.go @@ -0,0 +1,701 @@ +package strategy + +// Lefthook integration. +// +// Lefthook owns .git/hooks/* and regenerates every hook from its config on +// each run, gated by a checksum of that config. So Entire cannot keep a +// wrapper there: any lefthook.yml edit, or the `lefthook install -f` that its +// npm postinstall runs, silently takes the file back. Worse, the reclaim +// happens at the top of whatever hook fires next — typically the pre-commit of +// the very commit being made — so repairing at turn start loses the race. +// +// The fix is to stop wanting the file. Entire writes a config of its own and +// registers it with one `extends` entry in Lefthook's local config; Lefthook +// then invokes Entire's scripts itself. Lefthook resolves `extends` at run +// time, so the artifacts take effect immediately — no `lefthook install`, no +// user action — and Lefthook's own config is never touched. +// +// Verified against lefthook 2.1.10: a config reached via `extends` can declare +// `scripts`, those scripts receive git's hook arguments (pre-push gets the +// remote and its URL), and the whole arrangement survives both +// `lefthook install -f` and a config edit. + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/gitdir" + "github.com/entireio/cli/cmd/entire/cli/jsonutil" + "github.com/entireio/cli/cmd/entire/cli/osroot" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/worktreedir" + "gopkg.in/yaml.v3" +) + +const ( + // entireLefthookConfig is Entire's own Lefthook config. Entire owns this + // file outright, which is what keeps it out of the user's. + entireLefthookConfig = "entire-lefthook.yml" + // lefthookScriptDir is Lefthook's clone-local script directory, its + // default for source_dir_local. + lefthookScriptDir = ".lefthook-local" + lefthookScript = "entire.sh" + // lefthookOwnedMarker identifies a file as Entire's. Removal and + // overwriting both require it: the path is Entire's choice, but the + // repository is the user's. + lefthookOwnedMarker = "entire-cli-owned:lefthook:v1" +) + +// ErrLefthookLocalConfigUnwritable reports a local config Entire will not +// edit. Lefthook reads exactly one local config and lefthook-local.yml takes +// precedence over lefthook-local.toml, so creating the former beside a user's +// latter would silently stop their hooks running. Entire declines instead and +// leaves its native hooks in place. +var ErrLefthookLocalConfigUnwritable = errors.New("lefthook local config is not YAML") + +// ErrLefthookArtifactBlocked reports a path Entire cannot claim because an +// earlier backup is already sitting beside it. Burying that backup would +// destroy whatever it holds, so the integration stops and says so instead — +// unlike ErrLefthookLocalConfigUnwritable, which is a deliberate decision not +// to touch the repo and is handled silently. +var ErrLefthookArtifactBlocked = errors.New("lefthook artifact path is blocked by an existing backup") + +// lefthookLocalConfigNames are the local config names Lefthook reads, in its +// own precedence order. Writing into anything but the first match present +// would be a silent no-op. +var lefthookLocalConfigNames = []string{ + "lefthook-local.yml", "lefthook-local.yaml", + "lefthook-local.json", "lefthook-local.toml", + ".lefthook-local.yml", ".lefthook-local.yaml", + ".lefthook-local.json", ".lefthook-local.toml", +} + +// LefthookManaged reports whether this repository is managed by Lefthook. +// +// Only a main config counts. A lone lefthook-local.* is deliberately not +// evidence: Entire creates lefthook-local.yml itself, so trusting it would +// leave a repo classified as Lefthook-managed after the user removed Lefthook +// — reporting "via Lefthook" in status while nothing ran Entire at all. +func LefthookManaged(repoRoot string) bool { + for _, prefix := range []string{"", "."} { + for _, ext := range []string{"yml", "yaml", "json", "toml"} { + if _, err := os.Stat(filepath.Join(repoRoot, prefix+"lefthook."+ext)); err == nil { + return true + } + } + } + return false +} + +// EnsureLefthookIntegration registers Entire with Lefthook, returning the +// number of artifacts written. +// +// There is deliberately no rollback. Every write is atomic and content- +// idempotent, and the entry that activates the integration is written last, so +// a failure part way through leaves inert files that the next install +// overwrites. EnsureSetup runs at every turn start, so "the next install" is a +// guarantee rather than a hope. +func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, error) { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return 0, fmt.Errorf("resolve worktree root: %w", err) + } + root, err := worktreedir.OpenAt(repoRoot) + if err != nil { + return 0, fmt.Errorf("open worktree: %w", err) + } + cmdPrefix, err := hookCmdPrefix(absolutePath) + if err != nil { + return 0, err + } + + written := 0 + for _, spec := range buildHookSpecs(cmdPrefix) { + name := lefthookScriptPath(spec.name) + if err := displaceUnowned(root, name, 0o755); err != nil { + return 0, err + } + if err := osroot.MkdirAllNoSymlink(root, filepath.ToSlash(filepath.Dir(name)), 0o755); err != nil { + return 0, fmt.Errorf("create %s: %w", filepath.Dir(name), err) + } + changed, writeErr := writeHookFile(root, name, renderLefthookScript(spec)) + if writeErr != nil { + return 0, writeErr + } + if changed { + written++ + } + } + + if err := displaceUnowned(root, entireLefthookConfig, 0o644); err != nil { + return 0, err + } + changed, err := writeOwnedConfig(root) + if err != nil { + return 0, err + } + if changed { + written++ + } + + // Last: this is what makes Lefthook load any of the above. + if _, err := ensureExtendsEntry(root); err != nil { + return 0, err + } + if err := excludeArtifacts(ctx, repoRoot); err != nil { + return 0, err + } + if err := reconcileHookFiles(ctx, repoRoot); err != nil { + return 0, err + } + return written, nil +} + +// artifactsExcluded reports whether the exclude block is in place, which the +// install treats as part of being current. +func artifactsExcluded(ctx context.Context, repoRoot string) bool { + root, err := openGitCommonDir(ctx, repoRoot) + if err != nil { + return true // cannot tell; do not churn the install on it + } + data, err := osroot.ReadFileNoFollow(root, gitExcludePath) + return err == nil && strings.Contains(string(data), lefthookExcludeBlock()) +} + +// lefthookExcludeBlock is the exact text excludeArtifacts writes, so +// artifactsExcluded tests for what an install would produce and any future +// change to the entries repairs itself on the next turn. +func lefthookExcludeBlock() string { + // lefthook-local.yml is included because Entire creates it when absent and + // Lefthook documents it as a personal, uncommitted file. An exclude entry + // only affects untracked paths, so a repository that does commit it is + // unaffected by this. + return excludeBlockBegin + + "/" + entireLefthookConfig + "\n" + + "/" + lefthookScriptDir + "/\n" + + "/" + lefthookLocalConfigNames[0] + "\n" + + excludeBlockEnd +} + +const ( + // LefthookManagerName is how Lefthook is named in user-facing output and + // in HookDelivery.Manager; lefthookExtendsKey is Lefthook's own config key + // for pulling in another config file at run time. + LefthookManagerName = "Lefthook" + lefthookExtendsKey = "extends" + + gitExcludePath = "info/exclude" + excludeBlockBegin = "# " + lefthookOwnedMarker + " begin\n" + excludeBlockEnd = "# " + lefthookOwnedMarker + " end\n" +) + +// excludeArtifacts adds Entire's generated files to the clone-local +// .git/info/exclude. They are generated and per-clone, so they do not belong +// in the user's .gitignore — and left unignored they make a dirty worktree the +// normal state in every Lefthook repo, which eventually gets Entire's +// integration files committed by an agent that sees them in git status. +func excludeArtifacts(ctx context.Context, repoRoot string) error { + return rewriteExcludeBlock(ctx, repoRoot, lefthookExcludeBlock()) +} + +// rewriteExcludeBlock replaces Entire's marked block in .git/info/exclude with +// block, which is empty on uninstall. Failing to open the git dir is not an +// error: ignoring these files is a convenience, not part of the integration. +func rewriteExcludeBlock(ctx context.Context, repoRoot, block string) error { + root, err := openGitCommonDir(ctx, repoRoot) + if err != nil { + return nil //nolint:nilerr // best effort: ignoring is a convenience + } + existing, err := osroot.ReadFileNoFollow(root, gitExcludePath) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("read git exclude: %w", err) + } + body := string(existing) + if block != "" && strings.Contains(body, block) { + return nil + } + // Replace a previous block rather than appending a second one. + if i := strings.Index(body, excludeBlockBegin); i >= 0 { + if j := strings.Index(body[i:], excludeBlockEnd); j >= 0 { + body = body[:i] + body[i+j+len(excludeBlockEnd):] + } else { + return nil // an unterminated block is not ours to guess at + } + } else if block == "" { + return nil + } + if body != "" && !strings.HasSuffix(body, "\n") { + body += "\n" + } + if err := osroot.MkdirAllNoSymlink(root, "info", 0o755); err != nil { + return fmt.Errorf("create info dir: %w", err) + } + if err := jsonutil.WriteFileAtomicIn(root, gitExcludePath, []byte(body+block), 0o644); err != nil { + return fmt.Errorf("write git exclude: %w", err) + } + return nil +} + +func openGitCommonDir(ctx context.Context, repoRoot string) (*os.Root, error) { + commonDir, err := gitdir.CommonDirForWorktree(ctx, repoRoot) + if err != nil { + return nil, fmt.Errorf("resolve git common dir: %w", err) + } + root, err := gitdir.OpenAt(commonDir) + if err != nil { + return nil, fmt.Errorf("open git common dir: %w", err) + } + return root, nil +} + +// LefthookIntegrationCurrent reports whether Entire's artifacts are present +// and current: its config, the extends entry pointing at it, and every script. +func LefthookIntegrationCurrent(ctx context.Context, absolutePath bool) (bool, error) { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return false, fmt.Errorf("resolve worktree root: %w", err) + } + root, err := worktreedir.OpenAt(repoRoot) + if err != nil { + return false, fmt.Errorf("open worktree: %w", err) + } + want, err := renderOwnedConfig() + if err != nil { + return false, err + } + data, err := osroot.ReadFileNoFollow(root, entireLefthookConfig) + if err != nil || string(data) != want { + return false, nil //nolint:nilerr // a missing or stale config is "not current", not a failure + } + if !extendsEntryPresent(root) { + return false, nil + } + cmdPrefix, err := hookCmdPrefix(absolutePath) + if err != nil { + return false, err + } + for _, spec := range buildHookSpecs(cmdPrefix) { + content, err := osroot.ReadFileNoFollow(root, lefthookScriptPath(spec.name)) + if err != nil || string(content) != renderLefthookScript(spec) { + return false, nil //nolint:nilerr // same: stale, not broken + } + } + // The exclude entry counts as part of the install. Without it every + // Lefthook worktree reads as dirty, and a repo that predates this check + // needs a repair pass to pick it up. + return artifactsExcluded(ctx, repoRoot), nil +} + +// RemoveLefthookIntegration removes Entire's artifacts, returning the number +// removed. Files without Entire's marker are left alone. +func RemoveLefthookIntegration(ctx context.Context) (int, error) { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return 0, fmt.Errorf("resolve worktree root: %w", err) + } + root, err := worktreedir.OpenAt(repoRoot) + if err != nil { + return 0, fmt.Errorf("open worktree: %w", err) + } + removed := 0 + for _, hook := range gitHookNames { + name := lefthookScriptPath(hook) + owned, err := fileIsOwned(root, name) + if err != nil || !owned { + continue // unreadable or not ours: leave it + } + if err := osroot.RemoveNoSymlinks(root, name); err != nil && !os.IsNotExist(err) { + return removed, fmt.Errorf("remove %s: %w", name, err) + } + removed++ + } + if owned, ownErr := fileIsOwned(root, entireLefthookConfig); ownErr == nil && owned { + if err := osroot.RemoveNoSymlinks(root, entireLefthookConfig); err != nil && !os.IsNotExist(err) { + return removed, fmt.Errorf("remove %s: %w", entireLefthookConfig, err) + } + removed++ + } + dropped, err := removeExtendsEntry(root) + if err != nil { + return removed, err + } + if dropped { + removed++ + } + if err := rewriteExcludeBlock(ctx, repoRoot, ""); err != nil { + return removed, err + } + // Prune the directories the scripts lived in. RemoveNoSymlinks fails on a + // non-empty directory, which is exactly the guard wanted: anything the + // user put there keeps its parent alive. + for _, hook := range gitHookNames { + //nolint:errcheck // a non-empty directory is meant to survive + _ = osroot.RemoveNoSymlinks(root, filepath.ToSlash(filepath.Join(lefthookScriptDir, hook))) + } + //nolint:errcheck // same + _ = osroot.RemoveNoSymlinks(root, lefthookScriptDir) + return removed, nil +} + +func lefthookScriptPath(hook string) string { + return filepath.ToSlash(filepath.Join(lefthookScriptDir, hook, lefthookScript)) +} + +// renderLefthookScript is the native hook body with Entire's marker swapped +// for the Lefthook-owned one, so ownership is checkable and the two kinds of +// artifact are never confused for each other. +func renderLefthookScript(spec hookSpec) string { + return strings.Replace(spec.content, + "# "+entireHookMarker+"\n", "# "+lefthookOwnedMarker+"\n", 1) +} + +func renderOwnedConfig() (string, error) { + hooks := map[string]any{} + for _, hook := range gitHookNames { + hooks[hook] = map[string]any{ + "scripts": map[string]any{lefthookScript: map[string]any{"runner": "bash"}}, + } + } + hooks["source_dir_local"] = lefthookScriptDir + out, err := yaml.Marshal(hooks) + if err != nil { + return "", fmt.Errorf("render %s: %w", entireLefthookConfig, err) + } + return "# " + lefthookOwnedMarker + "\n" + + "# Managed by Entire. Remove the extends entry in your Lefthook local\n" + + "# config to detach; edits here are overwritten.\n" + string(out), nil +} + +func writeOwnedConfig(root *os.Root) (bool, error) { + content, err := renderOwnedConfig() + if err != nil { + return false, err + } + existing, readErr := osroot.ReadFileNoFollow(root, entireLefthookConfig) + if readErr == nil && string(existing) == content { + return false, nil + } + if err := jsonutil.WriteFileAtomicIn(root, entireLefthookConfig, []byte(content), 0o644); err != nil { + return false, fmt.Errorf("write %s: %w", entireLefthookConfig, err) + } + return true, nil +} + +func fileIsOwned(root *os.Root, name string) (bool, error) { + data, err := osroot.ReadFileNoFollow(root, name) + if err != nil { + return false, err //nolint:wrapcheck // callers only test the bool + } + return strings.Contains(string(data), lefthookOwnedMarker), nil +} + +// displaceUnowned moves a file Entire did not write aside to .pre-entire +// before Entire claims the path, and refuses rather than bury an earlier +// backup. Corruption destroys the marker, so without this a damaged script +// would read as a foreign file and could never be repaired. +func displaceUnowned(root *os.Root, name string, mode os.FileMode) error { + data, err := osroot.ReadFileNoFollow(root, name) + if err != nil || strings.Contains(string(data), lefthookOwnedMarker) { + return nil //nolint:nilerr // absent or ours: nothing to displace + } + backup := name + GitHookBackupSuffix + if _, err := osroot.ReadFileNoFollow(root, backup); err == nil { + return fmt.Errorf("%w: %s already exists; remove it to let Entire reinstall %s", + ErrLefthookArtifactBlocked, backup, name) + } + if err := jsonutil.WriteFileAtomicIn(root, backup, data, mode); err != nil { + return fmt.Errorf("back up %s: %w", name, err) + } + return nil +} + +// findLocalConfig returns the local config Lefthook actually reads, its +// contents, and whether Entire may write to it. +func findLocalConfig(root *os.Root) (name string, data []byte, err error) { + for _, candidate := range lefthookLocalConfigNames { + body, readErr := osroot.ReadFileNoFollow(root, candidate) + if readErr != nil { + continue + } + if !strings.HasSuffix(candidate, ".yml") && !strings.HasSuffix(candidate, ".yaml") { + return candidate, body, fmt.Errorf("%w: %s", ErrLefthookLocalConfigUnwritable, candidate) + } + return candidate, body, nil + } + return lefthookLocalConfigNames[0], nil, nil +} + +// ensureExtendsEntry adds one entry to the local config's `extends` sequence, +// preserving everything else including comments. Returns the config written. +func ensureExtendsEntry(root *os.Root) (string, error) { + name, existing, err := findLocalConfig(root) + if err != nil { + return name, err + } + doc, seq, err := extendsSequence(existing) + if err != nil { + return name, fmt.Errorf("%s: %w", name, err) + } + for _, item := range seq.Content { + if item.Value == entireLefthookConfig { + return name, nil + } + } + seq.Content = append(seq.Content, &yaml.Node{ + Kind: yaml.ScalarNode, Tag: "!!str", Value: entireLefthookConfig, + LineComment: lefthookOwnedMarker, + }) + out, err := encodeYAML(doc) + if err != nil { + return name, fmt.Errorf("%s: %w", name, err) + } + if err := jsonutil.WriteFileAtomicIn(root, name, out, 0o644); err != nil { + return name, fmt.Errorf("write %s: %w", name, err) + } + return name, nil +} + +// extendsEntryPresent reports whether the local config already pulls in +// Entire's config. An unwritable, absent or unparseable config has no entry. +func extendsEntryPresent(root *os.Root) bool { + _, existing, err := findLocalConfig(root) + if err != nil || existing == nil { + return false + } + _, seq, err := extendsSequence(existing) + if err != nil { + return false + } + for _, item := range seq.Content { + if item.Value == entireLefthookConfig { + return true + } + } + return false +} + +func removeExtendsEntry(root *os.Root) (bool, error) { + name, existing, err := findLocalConfig(root) + if err != nil || existing == nil { + return false, nil //nolint:nilerr // nothing of ours to remove + } + doc, seq, err := extendsSequence(existing) + if err != nil { + return false, nil //nolint:nilerr // unparseable: not our entry + } + kept := make([]*yaml.Node, 0, len(seq.Content)) + changed := false + for _, item := range seq.Content { + if item.Value == entireLefthookConfig { + changed = true + continue + } + kept = append(kept, item) + } + if !changed { + return false, nil + } + seq.Content = kept + mapping := doc.Content[0] + if len(kept) == 0 { + for i := 0; i+1 < len(mapping.Content); i += 2 { + if mapping.Content[i].Value == lefthookExtendsKey { + mapping.Content = append(mapping.Content[:i], mapping.Content[i+2:]...) + break + } + } + } + // A file that held nothing but our entry is ours to delete. + if len(mapping.Content) == 0 { + if err := osroot.RemoveNoSymlinks(root, name); err != nil && !os.IsNotExist(err) { + return false, fmt.Errorf("remove %s: %w", name, err) + } + return true, nil + } + out, err := encodeYAML(doc) + if err != nil { + return false, fmt.Errorf("%s: %w", name, err) + } + if err := jsonutil.WriteFileAtomicIn(root, name, out, 0o644); err != nil { + return false, fmt.Errorf("write %s: %w", name, err) + } + return true, nil +} + +// extendsSequence returns the document and its `extends` sequence, creating +// the key when absent. Kept as yaml.Node rather than a struct because the +// user's comments and key order must survive the round trip. +func extendsSequence(existing []byte) (*yaml.Node, *yaml.Node, error) { + doc := &yaml.Node{} + if len(strings.TrimSpace(string(existing))) == 0 { + doc.Kind = yaml.DocumentNode + doc.Content = []*yaml.Node{{Kind: yaml.MappingNode, Tag: "!!map"}} + } else if err := yaml.Unmarshal(existing, doc); err != nil { + return nil, nil, fmt.Errorf("parse: %w", err) + } + if len(doc.Content) != 1 || doc.Content[0].Kind != yaml.MappingNode { + return nil, nil, errors.New("parse: top level must be a mapping") + } + mapping := doc.Content[0] + found := -1 + for i := 0; i+1 < len(mapping.Content); i += 2 { + if mapping.Content[i].Value != lefthookExtendsKey { + continue + } + if found >= 0 { + return nil, nil, errors.New("duplicate extends key") + } + found = i + } + if found >= 0 { + seq := mapping.Content[found+1] + if seq.Kind != yaml.SequenceNode { + return nil, nil, errors.New("extends must be a sequence") + } + return doc, seq, nil + } + seq := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"} + mapping.Content = append(mapping.Content, + &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: lefthookExtendsKey}, seq) + return doc, seq, nil +} + +func encodeYAML(doc *yaml.Node) ([]byte, error) { + var out strings.Builder + enc := yaml.NewEncoder(&out) + enc.SetIndent(2) + if err := enc.Encode(doc); err != nil { + return nil, fmt.Errorf("encode: %w", err) + } + if err := enc.Close(); err != nil { + return nil, fmt.Errorf("encode: %w", err) + } + return []byte(out.String()), nil +} + +// lefthookLauncherMarker appears in every hook file Lefthook generates. It is +// how Entire recognises a hook path Lefthook has taken over. +const lefthookLauncherMarker = "call_lefthook" + +// reconcileHookFiles hands .git/hooks/* back to Lefthook and clears the +// wreckage of the era when the two fought over those files. +// +// Once Entire is registered in Lefthook's config, Lefthook's own hook runs +// Entire. Entire owning the hook file as well means the hook runs Entire +// TWICE — its own copy, then Lefthook's launcher via the chain call — so where +// Lefthook's launcher is sitting in Entire's backup it is moved back over +// Entire's hook. installSkipsHook then leaves that path alone. +// +// The .old files are not merely untidy: Lefthook refuses to move a hook +// aside when its backup already exists, and reports "could not replace the +// hook: can't rename pre-push to pre-push.old - file already exists" on every +// sync until it is gone. That error outlives this fix unless cleared, and +// every repo the fight has already touched is in exactly that state. +// +// Nothing is touched unless its contents prove whose it was. +func reconcileHookFiles(ctx context.Context, repoRoot string) error { + hooksDir, err := getHooksDirInPath(ctx, repoRoot) + if err != nil { + return nil //nolint:nilerr // best effort: no hooks dir, nothing to do + } + root, err := hooksRootForRemoval(hooksDir) + if err != nil { + return nil //nolint:nilerr // same + } + for _, hook := range gitHookNames { + backup := hook + GitHookBackupSuffix + if fileContains(root, backup, lefthookLauncherMarker) && fileContains(root, hook, entireHookMarker) { + if err := root.Rename(backup, hook); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("restore %s: %w", hook, err) + } + continue + } + // What Lefthook displaced, back when Entire owned the file. + stale := hook + ".old" + if !fileContains(root, stale, entireHookMarker) { + continue + } + if err := osroot.RemoveNoSymlinks(root, stale); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove stale backup %s: %w", stale, err) + } + } + return nil +} + +func fileContains(root *os.Root, name, marker string) bool { + data, err := osroot.ReadFileNoFollow(root, name) + return err == nil && strings.Contains(string(data), marker) +} + +// installSkipsHook reports whether InstallGitHook should leave a hook file +// alone because Lefthook owns it and already dispatches Entire from its own +// config. Writing Entire's hook over Lefthook's launcher would both run Entire +// twice and stop the user's own Lefthook commands until Lefthook's next sync. +// +// A hook Lefthook has not taken over is installed normally: in a repo where +// nobody has run `lefthook install` yet, Entire's own hooks are the only +// delivery there is. +func installSkipsHook(root *os.Root, hook string, lefthookDelivers bool) bool { + return lefthookDelivers && fileContains(root, hook, lefthookLauncherMarker) +} + +// lefthookDeliversHooks reports whether Lefthook will run Entire from its own +// configuration, which is what makes Entire's own hook files redundant. +func lefthookDeliversHooks(ctx context.Context, absolutePath bool) bool { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil || !LefthookManaged(repoRoot) { + return false + } + current, err := LefthookIntegrationCurrent(ctx, absolutePath) + return err == nil && current +} + +// HookDelivery describes whether Entire's Git hooks will actually fire. +type HookDelivery struct { + // OK is true when capture and delivery are wired up. + OK bool + // Manager names the hook manager that owns the hook files, if any. + Manager string + // Reason explains a false OK, ready to show a user. + Reason string +} + +// CheckHookDelivery reports whether Entire's hooks will fire in this +// repository. This is the question `entire status` was answering wrongly in +// #2264: it reported a checkpoint destination while no hook existed to reach +// it. There is no network access and no mutation. +// +// A Lefthook repo is judged on the integration, not on .git/hooks/*: Lefthook +// owns those files and rewrites them constantly, so their contents say nothing +// about whether Entire will run. +func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return HookDelivery{Reason: "Could not resolve the repository root."} + } + if LefthookManaged(repoRoot) { + current, checkErr := LefthookIntegrationCurrent(ctx, absolutePath) + switch { + case checkErr != nil: + return HookDelivery{Manager: LefthookManagerName, + Reason: fmt.Sprintf("Could not inspect Entire's Lefthook integration: %v", checkErr)} + case current: + return HookDelivery{OK: true, Manager: LefthookManagerName} + default: + return HookDelivery{Manager: LefthookManagerName, + Reason: "Entire is not registered with Lefthook, so Lefthook's hooks do not run it."} + } + } + switch CheckGitHookState(ctx) { + case GitHooksCurrent: + return HookDelivery{OK: true} + case GitHooksOutdated: + return HookDelivery{Reason: "Entire's Git hooks are outdated."} + case GitHooksAbsent: + } + return HookDelivery{Reason: "Entire's Git hooks are not installed."} +} diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go new file mode 100644 index 0000000000..aa20e00682 --- /dev/null +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -0,0 +1,428 @@ +package strategy + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/stretchr/testify/require" +) + +// newLefthookRepo is an isolated repository that looks Lefthook-managed. +// mainConfig names its Lefthook config; empty means lefthook.yml. +func newLefthookRepo(t *testing.T, mainConfig string) string { + t.Helper() + dir := t.TempDir() + testutil.InitRepo(t, dir) + if mainConfig == "" { + mainConfig = "lefthook.yml" + } + require.NoError(t, os.WriteFile(filepath.Join(dir, mainConfig), []byte("pre-commit: {}\n"), 0o644)) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + return dir +} + +// Install writes Entire's own config, one extends entry, and a script per +// hook — and nothing into Lefthook's own config. +func TestEnsureLefthookIntegration(t *testing.T) { + dir := newLefthookRepo(t, "") + mainBefore, err := os.ReadFile(filepath.Join(dir, "lefthook.yml")) + require.NoError(t, err) + + written, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + require.Positive(t, written) + + owned, err := os.ReadFile(filepath.Join(dir, entireLefthookConfig)) + require.NoError(t, err) + require.Contains(t, string(owned), lefthookOwnedMarker) + require.Contains(t, string(owned), "source_dir_local: "+lefthookScriptDir) + + local, err := os.ReadFile(filepath.Join(dir, "lefthook-local.yml")) + require.NoError(t, err) + require.Contains(t, string(local), entireLefthookConfig) + require.Equal(t, 1, strings.Count(string(local), entireLefthookConfig)) + + for _, hook := range gitHookNames { + script, err := os.ReadFile(filepath.Join(dir, lefthookScriptPath(hook))) + require.NoError(t, err, hook) + require.Contains(t, string(script), lefthookOwnedMarker, hook) + require.Contains(t, string(script), "hooks git "+hook, hook) + info, err := os.Stat(filepath.Join(dir, lefthookScriptPath(hook))) + require.NoError(t, err) + require.NotZero(t, info.Mode().Perm()&0o111, "%s must be executable", hook) + } + + mainAfter, err := os.ReadFile(filepath.Join(dir, "lefthook.yml")) + require.NoError(t, err) + require.Equal(t, mainBefore, mainAfter, "Lefthook's own config must not be touched") + + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.True(t, current) + + // Repeat installs are no-ops. + written, err = EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + require.Zero(t, written) +} + +// Entire never reads Lefthook's main config, so its format and location are +// irrelevant — .toml and .config/ repos work like any other. +func TestEnsureLefthookIntegration_MainConfigFormatIsIrrelevant(t *testing.T) { + for _, name := range []string{"lefthook.toml", "lefthook.json", ".lefthook.yaml"} { + t.Run(name, func(t *testing.T) { + newLefthookRepo(t, name) + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.True(t, current) + }) + } +} + +// The extends entry goes into the user's local config without disturbing it. +func TestEnsureLefthookIntegration_PreservesLocalConfig(t *testing.T) { + for _, tc := range []struct { + name, local string + want []string + }{ + {"existing content", "# keep me\npre-commit:\n commands:\n mine:\n run: true\n", + []string{"# keep me", "mine"}}, + {"existing extends", "extends:\n - user.yml\n", []string{"user.yml"}}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := newLefthookRepo(t, "") + require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook-local.yml"), []byte(tc.local), 0o644)) + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + got, err := os.ReadFile(filepath.Join(dir, "lefthook-local.yml")) + require.NoError(t, err) + require.Contains(t, string(got), entireLefthookConfig) + for _, want := range tc.want { + require.Contains(t, string(got), want) + } + }) + } +} + +// lefthook-local.yml takes precedence over lefthook-local.toml, so creating +// the former beside a user's latter would silently stop their hooks running. +func TestEnsureLefthookIntegration_RefusesToShadowANonYAMLLocalConfig(t *testing.T) { + dir := newLefthookRepo(t, "") + tomlPath := filepath.Join(dir, "lefthook-local.toml") + original := []byte("[pre-commit.commands.mine]\nrun = \"true\"\n") + require.NoError(t, os.WriteFile(tomlPath, original, 0o644)) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.ErrorIs(t, err, ErrLefthookLocalConfigUnwritable) + + after, err := os.ReadFile(tomlPath) + require.NoError(t, err) + require.Equal(t, original, after) + _, statErr := os.Stat(filepath.Join(dir, "lefthook-local.yml")) + require.True(t, os.IsNotExist(statErr), "must not create a shadowing lefthook-local.yml") +} + +// A file Entire did not write is displaced, never destroyed. Corruption +// destroys the marker, so a damaged script would otherwise read as foreign and +// could never be repaired. +func TestEnsureLefthookIntegration_DisplacesUnownedFiles(t *testing.T) { + dir := newLefthookRepo(t, "") + scriptPath := filepath.Join(dir, lefthookScriptPath("pre-push")) + require.NoError(t, os.MkdirAll(filepath.Dir(scriptPath), 0o755)) + theirs := []byte("#!/bin/sh\necho mine\n") + require.NoError(t, os.WriteFile(scriptPath, theirs, 0o755)) + configPath := filepath.Join(dir, entireLefthookConfig) + theirConfig := []byte("# not Entire's\nkey: value\n") + require.NoError(t, os.WriteFile(configPath, theirConfig, 0o644)) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + backup, err := os.ReadFile(scriptPath + GitHookBackupSuffix) + require.NoError(t, err, "the displaced script must be kept") + require.Equal(t, theirs, backup) + backup, err = os.ReadFile(configPath + GitHookBackupSuffix) + require.NoError(t, err, "the displaced config must be kept") + require.Equal(t, theirConfig, backup) + + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.True(t, current, "and Entire's own artifacts installed over them") +} + +// Uninstall takes only what Entire wrote. +func TestRemoveLefthookIntegration(t *testing.T) { + dir := newLefthookRepo(t, "") + localPath := filepath.Join(dir, "lefthook-local.yml") + require.NoError(t, os.WriteFile(localPath, []byte("pre-commit:\n commands:\n mine:\n run: true\n"), 0o644)) + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + removed, err := RemoveLefthookIntegration(t.Context()) + require.NoError(t, err) + require.Positive(t, removed) + + _, statErr := os.Stat(filepath.Join(dir, entireLefthookConfig)) + require.True(t, os.IsNotExist(statErr)) + local, err := os.ReadFile(localPath) + require.NoError(t, err, "the user's local config must survive") + require.NotContains(t, string(local), entireLefthookConfig) + require.Contains(t, string(local), "mine") + + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.False(t, current) +} + +// A foreign file at Entire's config path is not Entire's to delete. +func TestRemoveLefthookIntegration_LeavesForeignFiles(t *testing.T) { + dir := newLefthookRepo(t, "") + configPath := filepath.Join(dir, entireLefthookConfig) + theirs := []byte("# not Entire's\nkey: value\n") + require.NoError(t, os.WriteFile(configPath, theirs, 0o644)) + + _, err := RemoveLefthookIntegration(t.Context()) + require.NoError(t, err) + after, err := os.ReadFile(configPath) + require.NoError(t, err) + require.Equal(t, theirs, after) +} + +// Two kinds of wreckage from the era when Entire and Lefthook fought over +// .git/hooks/*: Entire's hook chaining to Lefthook's displaced launcher (which +// runs Entire twice now that Lefthook dispatches it), and the .old +// backups that make Lefthook fail every later sync (#1349). +func TestEnsureLefthookIntegration_ReconcilesHookFiles(t *testing.T) { + dir := newLefthookRepo(t, "") + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + + launcher := []byte("#!/bin/sh\ncall_lefthook run \"pre-push\" \"$@\"\n") + entireHook := []byte("#!/bin/sh\n# " + entireHookMarker + "\nentire hooks git pre-push \"$1\"\n") + write := func(name string, body []byte) { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, name), body, 0o755)) + } + write("pre-push", entireHook) + write("pre-push"+GitHookBackupSuffix, launcher) + write("commit-msg.old", entireHook) + // Another tool's backup, and a foreign hook Entire displaced: neither is + // Entire's to touch. + theirs := []byte("#!/bin/sh\necho someone else\n") + write("post-commit.old", theirs) + write("post-rewrite"+GitHookBackupSuffix, theirs) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + got, err := os.ReadFile(filepath.Join(hooksDir, "pre-push")) + require.NoError(t, err) + require.Equal(t, launcher, got, "Lefthook's launcher must be restored over Entire's hook") + _, statErr := os.Stat(filepath.Join(hooksDir, "pre-push"+GitHookBackupSuffix)) + require.True(t, os.IsNotExist(statErr), "the backup is consumed by the restore") + + _, statErr = os.Stat(filepath.Join(hooksDir, "commit-msg.old")) + require.True(t, os.IsNotExist(statErr), "Entire's stale wrapper backup must be cleared") + + for name, want := range map[string][]byte{ + "post-commit.old": theirs, + "post-rewrite" + GitHookBackupSuffix: theirs, + } { + got, err := os.ReadFile(filepath.Join(hooksDir, name)) + require.NoError(t, err, name) + require.Equal(t, want, got, "%s belongs to someone else", name) + } + + // And the native install leaves the yielded hook alone rather than + // clobbering the launcher and running Entire twice. + ClearHooksDirCache() + _, err = ReinstallGitHooks(t.Context()) + require.NoError(t, err) + got, err = os.ReadFile(filepath.Join(hooksDir, "pre-push")) + require.NoError(t, err) + require.Equal(t, launcher, got, "InstallGitHook must skip a hook Lefthook owns") + got, err = os.ReadFile(filepath.Join(hooksDir, "post-commit")) + require.NoError(t, err) + require.Contains(t, string(got), entireHookMarker, + "a hook Lefthook has not taken over is still Entire's to install") +} + +// Delivery is judged on the integration in a Lefthook repo, because Lefthook +// owns .git/hooks/* and rewrites them constantly. +func TestCheckHookDelivery_Lefthook(t *testing.T) { + newLefthookRepo(t, "") + got := CheckHookDelivery(t.Context(), false) + require.False(t, got.OK, "not registered yet") + require.Equal(t, "Lefthook", got.Manager) + require.NotEmpty(t, got.Reason) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + got = CheckHookDelivery(t.Context(), false) + require.True(t, got.OK) + require.Equal(t, "Lefthook", got.Manager) + require.Empty(t, got.Reason) +} + +// In a repo with no hook manager, delivery is the native hook state. +func TestCheckHookDelivery_Native(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + ClearHooksDirCache() + + got := CheckHookDelivery(t.Context(), false) + require.False(t, got.OK) + require.Empty(t, got.Manager) + + if _, err := ReinstallGitHooks(t.Context()); err != nil { + t.Fatalf("ReinstallGitHooks: %v", err) + } + ClearHooksDirCache() + got = CheckHookDelivery(t.Context(), false) + require.True(t, got.OK) + require.Empty(t, got.Manager) +} + +// Entire's artifacts are generated and per-clone, so they go in +// .git/info/exclude rather than the user's .gitignore — and a stale block from +// an earlier version is rewritten rather than appended to. +func TestEnsureLefthookIntegration_ExcludesArtifacts(t *testing.T) { + dir := newLefthookRepo(t, "") + excludePath := filepath.Join(dir, ".git", "info", "exclude") + require.NoError(t, os.MkdirAll(filepath.Dir(excludePath), 0o755)) + require.NoError(t, os.WriteFile(excludePath, + []byte("# user's own\n/scratch\n"+excludeBlockBegin+"/stale-entry\n"+excludeBlockEnd), 0o644)) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + got, err := os.ReadFile(excludePath) + require.NoError(t, err) + require.Contains(t, string(got), "/scratch", "the user's own entries must survive") + require.NotContains(t, string(got), "/stale-entry", "the previous block must be replaced") + require.Equal(t, 1, strings.Count(string(got), excludeBlockBegin), "exactly one block") + for _, entry := range []string{ + "/" + entireLefthookConfig, + "/" + lefthookScriptDir + "/", + "/" + lefthookLocalConfigNames[0], + } { + require.Contains(t, string(got), entry+"\n") + } + + // An install that predates an entry is not "current", so it repairs itself. + require.NoError(t, os.WriteFile(excludePath, + []byte(excludeBlockBegin+"/"+entireLefthookConfig+"\n"+excludeBlockEnd), 0o644)) + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.False(t, current, "a partial exclude block must trigger a repair") + + _, err = EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + current, err = LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.True(t, current) + + // Uninstall takes the block back out and leaves the user's entries. + _, err = RemoveLefthookIntegration(t.Context()) + require.NoError(t, err) + got, err = os.ReadFile(excludePath) + require.NoError(t, err) + require.NotContains(t, string(got), excludeBlockBegin) + require.NotContains(t, string(got), entireLefthookConfig) + + // And the script directory it created, since nothing else is in it. + _, statErr := os.Stat(filepath.Join(dir, lefthookScriptDir)) + require.True(t, os.IsNotExist(statErr), "the empty script dir must be pruned") +} + +// Entire creates lefthook-local.yml itself, so it must never be the thing that +// makes a repo look Lefthook-managed — otherwise removing Lefthook leaves +// status claiming delivery "via Lefthook" with nothing running Entire. +func TestLefthookManaged(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + file string + want bool + }{ + {"lefthook.yml", true}, + {".lefthook.yaml", true}, + {"lefthook.toml", true}, + {"lefthook-local.yml", false}, + {".lefthook-local.toml", false}, + {"package.json", false}, + } { + t.Run(tc.file, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, tc.file), []byte(""), 0o644)) + require.Equal(t, tc.want, LefthookManaged(dir)) + }) + } +} + +// Entire will not bury an existing backup to claim a path: that backup is the +// only copy of whatever was displaced before. +func TestEnsureLefthookIntegration_RefusesToBuryABackup(t *testing.T) { + dir := newLefthookRepo(t, "") + scriptPath := filepath.Join(dir, lefthookScriptPath("pre-push")) + require.NoError(t, os.MkdirAll(filepath.Dir(scriptPath), 0o755)) + require.NoError(t, os.WriteFile(scriptPath, []byte("#!/bin/sh\necho newer\n"), 0o755)) + older := []byte("#!/bin/sh\necho older\n") + require.NoError(t, os.WriteFile(scriptPath+GitHookBackupSuffix, older, 0o755)) + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.ErrorIs(t, err, ErrLefthookArtifactBlocked) + + got, err := os.ReadFile(scriptPath + GitHookBackupSuffix) + require.NoError(t, err) + require.Equal(t, older, got, "the existing backup must be untouched") +} + +// A visible install that installed nothing must not claim otherwise. +func TestInstallGitHook_ReportsLefthookDeliveryInsteadOfAFalseInstall(t *testing.T) { + dir := newLefthookRepo(t, "") + _, err := EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + for _, hook := range gitHookNames { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, hook), + []byte("#!/bin/sh\ncall_lefthook run \""+hook+"\" \"$@\"\n"), 0o755)) + } + ClearHooksDirCache() + + out := captureStdout(t, func() { + _, err := InstallGitHook(t.Context(), false, false) + require.NoError(t, err) + }) + require.Contains(t, out, "run through Lefthook") + require.NotContains(t, out, "Installed git hooks") +} + +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + require.NoError(t, err) + saved := os.Stdout + os.Stdout = w + defer func() { os.Stdout = saved }() + fn() + require.NoError(t, w.Close()) + data, err := io.ReadAll(r) + require.NoError(t, err) + return string(data) +} From 24835e281383e41bd74e7579a8eeab904da5c115 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 16:21:09 -0700 Subject: [PATCH 02/18] fix(hooks): leave the repo untouched when declining a Lefthook config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsureLefthookIntegration wrote its six artifacts and only then reached ensureExtendsEntry, so a repo with a non-YAML lefthook-local.* — the documented ErrLefthookLocalConfigUnwritable case — got entire-lefthook.yml and .lefthook-local/ written before the refusal, and excludeArtifacts never ran. Since ensureLefthookIntegrationIfManaged treats that refusal as a decision rather than a failure, it recurred on every turn rather than resolving itself: six untracked, un-excluded files in the worktree permanently, which is the accidental-commit risk this file's own comments warn about. Two changes, covering two different failures. The local config is probed before anything is written, so declining to shadow it now means touching nothing at all. And the exclude entries go in while the paths are still empty — they name paths, not existing files, so writing them first costs nothing and a write that dies part way through (a blocked artifact path, a full disk) can no longer leave anything unignored. Verifying that against real lefthook surfaced a second bug in the same repo shape: `doctor --force` exited with a raw "lefthook local config is not YAML" and `entire status` reported "Checkpoints are NOT being captured" — in a repo where Entire's own hooks are installed and firing. A declined integration is permanent, so there was no repair coming and no fix to offer. CheckHookDelivery now judges such a repo by its native hooks and reports HookDelivery.Declined, which doctor prints and status --json exposes as hooks_lefthook_declined, so the repo reads as working while still saying why Lefthook is not the one carrying it. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29CCR921GJWJSAJ1PB1WD04 --- cmd/entire/cli/doctor.go | 10 +++- cmd/entire/cli/status.go | 4 ++ cmd/entire/cli/strategy/lefthook.go | 62 +++++++++++++++++++++--- cmd/entire/cli/strategy/lefthook_test.go | 49 ++++++++++++++++++- 4 files changed, 114 insertions(+), 11 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index b4dfd1b59c..599d54a51d 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -637,7 +637,15 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { // their contents say nothing about whether Entire runs. Every other // manager only overwrites at install time, so for those the hook files // remain the answer and the check below is the right one. - if delivery := strategy.CheckHookDelivery(ctx, false); delivery.Manager == strategy.LefthookManagerName { + delivery := strategy.CheckHookDelivery(ctx, false) + if delivery.Declined != "" { + // Entire's own hooks are the arrangement here, so the checks below are + // the right ones — but say why Lefthook is not carrying it, since + // otherwise a Lefthook repo silently looks like any other. + fmt.Fprintf(w, "Note: Entire is not registered in Lefthook's config (%s is not YAML,\n", delivery.Declined) + fmt.Fprintln(w, " and Entire will not shadow it). Entire's own Git hooks are used instead.") + } + if delivery.Manager == strategy.LefthookManagerName { if delivery.OK { // --force also reconciles .git/hooks/*, which is where a repo left // mid-fight — Entire's own hook chaining to Lefthook's displaced diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index e06fc89a25..aa510a229e 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -1085,6 +1085,9 @@ type statusJSON struct { HooksDeliver bool `json:"hooks_deliver"` HooksManager string `json:"hooks_manager,omitempty"` HooksDeliverReason string `json:"hooks_deliver_reason,omitempty"` + // HooksLefthookDeclined names a Lefthook local config Entire will not + // write to, which is why Lefthook is not the one delivering. + HooksLefthookDeclined string `json:"hooks_lefthook_declined,omitempty"` // CheckpointReadSourceUnknown reports that the read-source probe failed, // so no read source could be determined. Emitted only alongside // checkpoint_push_disabled, and checkpoint_sync_remote is then absent @@ -1203,6 +1206,7 @@ func runStatusJSON(ctx context.Context, w io.Writer) error { result.HooksDeliver = delivery.OK result.HooksManager = delivery.Manager result.HooksDeliverReason = delivery.Reason + result.HooksLefthookDeclined = delivery.Declined result.CheckpointReadFallback = syncInfo.ReadFallback result.CheckpointReadSourceUnknown = syncInfo.ReadSourceUnknown result.UnpushedCheckpoints = syncInfo.Unpushed diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 26ef525d7b..49d5083978 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -99,6 +99,12 @@ func LefthookManaged(repoRoot string) bool { // a failure part way through leaves inert files that the next install // overwrites. EnsureSetup runs at every turn start, so "the next install" is a // guarantee rather than a hope. +// +// That argument only holds for a failure that eventually stops happening, so +// two things come before the writes: the non-YAML local config is refused up +// front, because it is a decision not to touch the repo rather than a failure +// that will pass, and the exclude entries go in while the paths are still +// empty, so a partial write is never left unignored. func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, error) { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil { @@ -108,11 +114,29 @@ func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, err if err != nil { return 0, fmt.Errorf("open worktree: %w", err) } + // Refuse BEFORE writing anything. Declining to shadow a non-YAML local + // config has to mean leaving the repo untouched: the extends entry is what + // makes the other artifacts do anything, so writing them first left six + // untracked files in the worktree that this same function had not reached + // the point of excluding — and because ensureLefthookIntegrationIfManaged + // treats this refusal as a decision rather than a failure, that recurred + // on every turn instead of resolving itself. + if _, _, err := findLocalConfig(root); err != nil { + return 0, err + } cmdPrefix, err := hookCmdPrefix(absolutePath) if err != nil { return 0, err } + // Before the writes, not after: a write that fails part way through (a + // blocked artifact path, a full disk) otherwise leaves files in the + // worktree that nothing has excluded yet, and the entries name paths + // rather than existing files, so writing them early costs nothing. + if err := excludeArtifacts(ctx, repoRoot); err != nil { + return 0, err + } + written := 0 for _, spec := range buildHookSpecs(cmdPrefix) { name := lefthookScriptPath(spec.name) @@ -146,9 +170,6 @@ func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, err if _, err := ensureExtendsEntry(root); err != nil { return 0, err } - if err := excludeArtifacts(ctx, repoRoot); err != nil { - return 0, err - } if err := reconcileHookFiles(ctx, repoRoot); err != nil { return 0, err } @@ -658,10 +679,15 @@ func lefthookDeliversHooks(ctx context.Context, absolutePath bool) bool { type HookDelivery struct { // OK is true when capture and delivery are wired up. OK bool - // Manager names the hook manager that owns the hook files, if any. + // Manager names the hook manager that delivers Entire, if one does. Manager string // Reason explains a false OK, ready to show a user. Reason string + // Declined names the Lefthook local config Entire will not write to, when + // that is why Lefthook is not the one delivering. It is set whether or not + // OK is true, because it is the answer to "why is Entire not in Lefthook's + // config" in a repository whose hooks are working fine without it. + Declined string } // CheckHookDelivery reports whether Entire's hooks will fire in this @@ -677,6 +703,7 @@ func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { if err != nil { return HookDelivery{Reason: "Could not resolve the repository root."} } + declined := "" if LefthookManaged(repoRoot) { current, checkErr := LefthookIntegrationCurrent(ctx, absolutePath) switch { @@ -685,17 +712,36 @@ func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { Reason: fmt.Sprintf("Could not inspect Entire's Lefthook integration: %v", checkErr)} case current: return HookDelivery{OK: true, Manager: LefthookManagerName} - default: + } + // A declined integration is permanent, not a repair pending: Entire's + // own hooks are the arrangement in such a repo, so the answer is the + // native one. Saying "not registered with Lefthook" here would report + // a working repository as broken, forever and with no fix to offer. + if declined = declinedLefthookLocalConfig(repoRoot); declined == "" { return HookDelivery{Manager: LefthookManagerName, Reason: "Entire is not registered with Lefthook, so Lefthook's hooks do not run it."} } } switch CheckGitHookState(ctx) { case GitHooksCurrent: - return HookDelivery{OK: true} + return HookDelivery{OK: true, Declined: declined} case GitHooksOutdated: - return HookDelivery{Reason: "Entire's Git hooks are outdated."} + return HookDelivery{Reason: "Entire's Git hooks are outdated.", Declined: declined} case GitHooksAbsent: } - return HookDelivery{Reason: "Entire's Git hooks are not installed."} + return HookDelivery{Reason: "Entire's Git hooks are not installed.", Declined: declined} +} + +// declinedLefthookLocalConfig names the local config Entire refuses to write +// to, or "" when there is none to refuse. +func declinedLefthookLocalConfig(repoRoot string) string { + root, err := worktreedir.OpenAt(repoRoot) + if err != nil { + return "" + } + name, _, err := findLocalConfig(root) + if errors.Is(err, ErrLefthookLocalConfigUnwritable) { + return name + } + return "" } diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index aa20e00682..4a16d7ee2b 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -128,8 +128,20 @@ func TestEnsureLefthookIntegration_RefusesToShadowANonYAMLLocalConfig(t *testing after, err := os.ReadFile(tomlPath) require.NoError(t, err) require.Equal(t, original, after) - _, statErr := os.Stat(filepath.Join(dir, "lefthook-local.yml")) - require.True(t, os.IsNotExist(statErr), "must not create a shadowing lefthook-local.yml") + + // Declining must leave the repo untouched. Writing the artifacts first and + // only then failing left them untracked and un-excluded — and the refusal + // is a decision rather than a transient failure, so every turn rewrote + // them and none of them ever reached excludeArtifacts. + for _, path := range []string{ + "lefthook-local.yml", + entireLefthookConfig, + lefthookScriptDir, + lefthookScriptPath("pre-push"), + } { + _, statErr := os.Stat(filepath.Join(dir, path)) + require.True(t, os.IsNotExist(statErr), "%s must not be created", path) + } } // A file Entire did not write is displaced, never destroyed. Corruption @@ -389,6 +401,13 @@ func TestEnsureLefthookIntegration_RefusesToBuryABackup(t *testing.T) { got, err := os.ReadFile(scriptPath + GitHookBackupSuffix) require.NoError(t, err) require.Equal(t, older, got, "the existing backup must be untouched") + + // This failure stops the install part way through, so whatever it did + // manage to write must already be excluded. + exclude, err := os.ReadFile(filepath.Join(dir, ".git", "info", "exclude")) + require.NoError(t, err) + require.Contains(t, string(exclude), lefthookExcludeBlock(), + "a partial install must not leave unignored artifacts") } // A visible install that installed nothing must not claim otherwise. @@ -426,3 +445,29 @@ func captureStdout(t *testing.T, fn func()) string { require.NoError(t, err) return string(data) } + +// A repo whose local config Entire will not write to is a permanent +// arrangement, not a repair pending: Entire's own hooks deliver there, so +// reporting "not registered with Lefthook" would call a working repository +// broken forever with no fix to offer. +func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testing.T) { + dir := newLefthookRepo(t, "") + require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook-local.toml"), + []byte("[pre-commit.commands.mine]\nrun = \"true\"\n"), 0o644)) + ClearHooksDirCache() + + got := CheckHookDelivery(t.Context(), false) + require.False(t, got.OK, "no hooks installed yet") + require.Empty(t, got.Manager, "Lefthook is not the one delivering") + require.Equal(t, "lefthook-local.toml", got.Declined) + require.Contains(t, got.Reason, "not installed", "the reason must be the native one") + + _, err := ReinstallGitHooks(t.Context()) + require.NoError(t, err) + ClearHooksDirCache() + + got = CheckHookDelivery(t.Context(), false) + require.True(t, got.OK, "Entire's own hooks deliver here") + require.Empty(t, got.Manager) + require.Equal(t, "lefthook-local.toml", got.Declined, "and the reason why is still reported") +} From 05d5da6f5d6a4b7e29b2a792385065be45cacb5d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 16:39:47 -0700 Subject: [PATCH 03/18] fix(hooks): cut the Lefthook turn-start cost, and uninstall in install's reverse order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the Lefthook integration. The turn-start step ran on every prompt with no gate, on a path EnsureSetup documents as synchronous and timed. Measured, the cost was not the file reads it looked like: LefthookIntegrationCurrent spent 11.2ms resolving the git common dir for the exclude check, and reconcileHookFiles another 13.0ms resolving the hooks dir — two `git rev-parse` subprocesses, the first on a currency check a single turn runs two or three times. The common dir is now memoized per worktree root (a worktree's does not move while the process lives, and only the path is cached — osroot.Shared already memoizes the root and is what keeps a handle from outliving a deleted directory), and reconcileHookFiles uses the already-cached GetHooksDir instead of the by-path variant. The whole step goes from 23.6ms to 0.42ms, against ~10-14ms for the worktree-root subprocess EnsureSetup already pays for. A non-Lefthook repo still exits after eight stats. RemoveLefthookIntegration deleted entire-lefthook.yml and its scripts before dropping the `extends` entry naming them, so an interruption or a failed config write left Lefthook's config pointing at a file that was gone — the reverse of the install, which deliberately writes that entry last. It now removes the entry first. Lefthook 2.1.10 tolerates a dangling extends (run, install, validate and dump all succeed, and the user's own commands still fire), so this is not repairing a break; it is declining to depend on another tool's tolerance, and it fails in the better direction, since giving up at that step now leaves the integration whole rather than half-deleted. The test helper clears both process-global caches, which are keyed by a directory each test has just left. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29DEVHFN0HX551G3RB0MCEM --- cmd/entire/cli/strategy/common.go | 2 +- cmd/entire/cli/strategy/lefthook.go | 71 ++++++++++++++++++++---- cmd/entire/cli/strategy/lefthook_test.go | 11 +++- 3 files changed, 70 insertions(+), 14 deletions(-) diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index 4b60519704..a3dfca9b3c 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -1837,7 +1837,7 @@ func ensureLefthookIntegrationIfManaged(ctx context.Context) error { return nil //nolint:nilerr // inspection failure falls back to native hooks } if current { - return reconcileHookFiles(ctx, repoRoot) + return reconcileHookFiles(ctx) } if _, err := EnsureLefthookIntegration(ctx, absolute); err != nil { if errors.Is(err, ErrLefthookLocalConfigUnwritable) { diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 49d5083978..185aa5dd94 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -27,6 +27,7 @@ import ( "os" "path/filepath" "strings" + "sync" "github.com/entireio/cli/cmd/entire/cli/gitdir" "github.com/entireio/cli/cmd/entire/cli/jsonutil" @@ -170,7 +171,7 @@ func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, err if _, err := ensureExtendsEntry(root); err != nil { return 0, err } - if err := reconcileHookFiles(ctx, repoRoot); err != nil { + if err := reconcileHookFiles(ctx); err != nil { return 0, err } return written, nil @@ -262,9 +263,9 @@ func rewriteExcludeBlock(ctx context.Context, repoRoot, block string) error { } func openGitCommonDir(ctx context.Context, repoRoot string) (*os.Root, error) { - commonDir, err := gitdir.CommonDirForWorktree(ctx, repoRoot) + commonDir, err := gitCommonDirFor(ctx, repoRoot) if err != nil { - return nil, fmt.Errorf("resolve git common dir: %w", err) + return nil, err } root, err := gitdir.OpenAt(commonDir) if err != nil { @@ -273,6 +274,42 @@ func openGitCommonDir(ctx context.Context, repoRoot string) (*os.Root, error) { return root, nil } +// gitCommonDirCache memoizes the git common dir per worktree root. Resolving +// it is a `git rev-parse` subprocess — measured at ~10ms, against ~0.3µs for +// the root open it feeds — and the exclude check that needs it is part of the +// currency test, which a single turn-start runs two or three times. A +// worktree's common dir does not move while the process lives. Only the path +// is cached; the root itself is already memoized by osroot.Shared, which is +// also what keeps a handle from outliving a deleted directory here. +var ( + gitCommonDirMu sync.RWMutex + gitCommonDirCache = map[string]string{} +) + +func gitCommonDirFor(ctx context.Context, repoRoot string) (string, error) { + gitCommonDirMu.RLock() + cached, ok := gitCommonDirCache[repoRoot] + gitCommonDirMu.RUnlock() + if ok { + return cached, nil + } + commonDir, err := gitdir.CommonDirForWorktree(ctx, repoRoot) + if err != nil { + return "", fmt.Errorf("resolve git common dir: %w", err) + } + gitCommonDirMu.Lock() + gitCommonDirCache[repoRoot] = commonDir + gitCommonDirMu.Unlock() + return commonDir, nil +} + +// clearGitCommonDirCache exists for tests that reuse a worktree root path. +func clearGitCommonDirCache() { + gitCommonDirMu.Lock() + gitCommonDirCache = map[string]string{} + gitCommonDirMu.Unlock() +} + // LefthookIntegrationCurrent reports whether Entire's artifacts are present // and current: its config, the extends entry pointing at it, and every script. func LefthookIntegrationCurrent(ctx context.Context, absolutePath bool) (bool, error) { @@ -322,7 +359,21 @@ func RemoveLefthookIntegration(ctx context.Context) (int, error) { if err != nil { return 0, fmt.Errorf("open worktree: %w", err) } + // The extends entry goes first, mirroring the install, which writes it + // last: at no point should Lefthook's config name a file that is not + // there. Lefthook 2.1.10 tolerates a dangling extends (run, install, + // validate and dump all succeed), so this is not repairing a break — it is + // declining to depend on another tool's tolerance, and it fails in the + // better direction, since giving up here leaves the integration whole + // rather than half-deleted. removed := 0 + dropped, err := removeExtendsEntry(root) + if err != nil { + return removed, err + } + if dropped { + removed++ + } for _, hook := range gitHookNames { name := lefthookScriptPath(hook) owned, err := fileIsOwned(root, name) @@ -340,13 +391,6 @@ func RemoveLefthookIntegration(ctx context.Context) (int, error) { } removed++ } - dropped, err := removeExtendsEntry(root) - if err != nil { - return removed, err - } - if dropped { - removed++ - } if err := rewriteExcludeBlock(ctx, repoRoot, ""); err != nil { return removed, err } @@ -618,8 +662,11 @@ const lefthookLauncherMarker = "call_lefthook" // every repo the fight has already touched is in exactly that state. // // Nothing is touched unless its contents prove whose it was. -func reconcileHookFiles(ctx context.Context, repoRoot string) error { - hooksDir, err := getHooksDirInPath(ctx, repoRoot) +func reconcileHookFiles(ctx context.Context) error { + // GetHooksDir rather than the by-path variant: both callers resolved their + // worktree root from this same process's directory, and this one is + // memoized. The by-path variant is another ~13ms `git rev-parse` per turn. + hooksDir, err := GetHooksDir(ctx) if err != nil { return nil //nolint:nilerr // best effort: no hooks dir, nothing to do } diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 4a16d7ee2b..e9fc07a5bf 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -23,8 +23,17 @@ func newLefthookRepo(t *testing.T, mainConfig string) string { } require.NoError(t, os.WriteFile(filepath.Join(dir, mainConfig), []byte("pre-commit: {}\n"), 0o644)) t.Chdir(dir) + // Both caches are process-global and keyed by the directory this test just + // left, so a stale entry would point the hooks-dir and common-dir lookups + // at another test's repository. paths.ClearWorktreeRootCache() - t.Cleanup(paths.ClearWorktreeRootCache) + ClearHooksDirCache() + clearGitCommonDirCache() + t.Cleanup(func() { + paths.ClearWorktreeRootCache() + ClearHooksDirCache() + clearGitCommonDirCache() + }) return dir } From 47c40ad1d4e7b618f779d20a0e76313866c58546 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 16:59:13 -0700 Subject: [PATCH 04/18] fix(hooks): require a hook file that reaches Entire, and verify Lefthook on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registering with Lefthook is not delivery. Git runs a hook only if the FILE exists, and Lefthook generates one per hook it knew about at its last install; Entire's config arrives through `extends` at run time, so a repo whose lefthook.yml declares only pre-commit has no file for the other four. Every artifact Entire owns can be present and correct while nothing dispatches Entire at all — and `entire status` reported hooks_deliver true in exactly that repo, which is the same false OK (#2264) this integration exists to remove. CheckHookDelivery now asks the question git asks: is there a file here, and does it lead to Entire — Lefthook's launcher, or Entire's own hook. `doctor --force` fills the gaps after registering, which the native install does by skipping the paths Lefthook already owns. Recognising Lefthook's launcher is now structural rather than a search for the word "lefthook". A false positive there is silent and permanent: the file is restored over Entire's hook by reconcileHookFiles and then skipped forever by installSkipsHook, so capture stops for that hook with nothing to repair it. The test is Lefthook's template shape — the call_lefthook function definition, plus the dispatch line for THIS hook — with a table covering the near misses, including a hand-written hook that runs lefthook itself. Windows was the one path in this integration that is not true by construction: Entire's hooks reach Lefthook as `scripts` with a bash runner, so Lefthook resolves bash through PATH and spawns it with a POSIX script on a platform whose shell is not POSIX. Two tests now cover it, named for ci.yml's windows-latest selector: one runs the rendered script under bash and checks the hook's arguments survive, and one drives the whole chain with the real Lefthook binary — git runs Lefthook's hook, Lefthook resolves the extends and spawns Entire's script. The Windows job installs lefthook 2.1.10 so the second is a test rather than a skip, and ENTIRE_TEST_REQUIRE_LEFTHOOK makes a failed install fail the job instead of passing silently. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29EJEGJGNNNFTYPX5DT5VYK --- .github/workflows/ci.yml | 12 +- cmd/entire/cli/doctor.go | 6 + cmd/entire/cli/status_test.go | 7 + cmd/entire/cli/strategy/lefthook.go | 64 ++++++++- cmd/entire/cli/strategy/lefthook_test.go | 92 ++++++++++-- .../strategy/lefthook_windows_runner_test.go | 131 ++++++++++++++++++ 6 files changed, 297 insertions(+), 15 deletions(-) create mode 100644 cmd/entire/cli/strategy/lefthook_windows_runner_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1ae7de95f2..9e1f2f5752 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,7 +78,7 @@ jobs: test-windows: runs-on: windows-latest - timeout-minutes: 10 + timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -86,9 +86,19 @@ jobs: - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod + # Lefthook owns .git/hooks/* and runs Entire's hooks as `scripts` with a + # bash runner, which is the one part of that integration Windows can + # break. Installing the real binary is what makes + # TestLefthookDeliversEntireOnWindows a test rather than a skip; + # ENTIRE_TEST_REQUIRE_LEFTHOOK turns a failed install into a failure + # instead of a silent pass. + - name: Install lefthook + run: go install github.com/evilmartians/lefthook/v2@v2.1.10 # Real-Windows tests must include Windows or MSYS in their top-level # test name so this job selects them without running POSIX-only harnesses. - name: Windows unit tests + env: + ENTIRE_TEST_REQUIRE_LEFTHOOK: "1" run: go test ./cmd/entire/cli/... -run '(Windows|MSYS)' -count=1 -v test: diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 599d54a51d..6a4b725f34 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -669,6 +669,12 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { if _, err := strategy.EnsureLefthookIntegration(ctx, false); err != nil { return fmt.Errorf("register Entire with Lefthook: %w", err) } + // Registering is not enough when Lefthook has no hook file for a hook + // — git runs nothing there. The native install fills exactly those + // gaps: it skips every path Lefthook already owns. + if _, err := strategy.ReinstallGitHooks(ctx); err != nil { + return fmt.Errorf("install git hooks alongside Lefthook: %w", err) + } fmt.Fprintln(w, " ✓ Re-registered") return nil } diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 66a4df9724..03a844efdb 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -3313,6 +3313,13 @@ func TestRunStatus_HookDeliveryLine(t *testing.T) { if _, err := strategy.EnsureLefthookIntegration(context.Background(), false); err != nil { t.Fatalf("EnsureLefthookIntegration() error = %v", err) } + // Registering is not delivery on its own: git runs a hook only if the file + // exists, and Lefthook creates one per hook it knew about at its last + // install. The native install fills the paths Lefthook has not taken. + if _, err := strategy.ReinstallGitHooks(context.Background()); err != nil { + t.Fatalf("ReinstallGitHooks() error = %v", err) + } + strategy.ClearHooksDirCache() stdout.Reset() if err := runStatus(context.Background(), &stdout, false, false); err != nil { diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 185aa5dd94..92eaa4af8f 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -642,9 +642,26 @@ func encodeYAML(doc *yaml.Node) ([]byte, error) { return []byte(out.String()), nil } -// lefthookLauncherMarker appears in every hook file Lefthook generates. It is -// how Entire recognises a hook path Lefthook has taken over. -const lefthookLauncherMarker = "call_lefthook" +// isLefthookLauncher reports whether a file is the hook Lefthook generates for +// this hook name. +// +// The test is deliberately structural rather than a search for the word +// "lefthook", because a false positive here is silent and permanent: a hook +// misread as Lefthook's gets restored over Entire's by reconcileHookFiles and +// then skipped forever by installSkipsHook, so capture stops for that hook and +// nothing ever repairs it. Lefthook's template defines a call_lefthook shell +// function and ends by dispatching THIS hook through it; a hand-written script +// that merely runs lefthook does neither, and a launcher for a different hook +// fails the second test. +func isLefthookLauncher(root *os.Root, name, hook string) bool { + data, err := osroot.ReadFileNoFollow(root, name) + if err != nil { + return false + } + body := string(data) + return strings.Contains(body, "call_lefthook()") && + strings.Contains(body, `call_lefthook run "`+hook+`"`) +} // reconcileHookFiles hands .git/hooks/* back to Lefthook and clears the // wreckage of the era when the two fought over those files. @@ -676,7 +693,7 @@ func reconcileHookFiles(ctx context.Context) error { } for _, hook := range gitHookNames { backup := hook + GitHookBackupSuffix - if fileContains(root, backup, lefthookLauncherMarker) && fileContains(root, hook, entireHookMarker) { + if isLefthookLauncher(root, backup, hook) && fileContains(root, hook, entireHookMarker) { if err := root.Rename(backup, hook); err != nil && !os.IsNotExist(err) { return fmt.Errorf("restore %s: %w", hook, err) } @@ -708,7 +725,39 @@ func fileContains(root *os.Root, name, marker string) bool { // nobody has run `lefthook install` yet, Entire's own hooks are the only // delivery there is. func installSkipsHook(root *os.Root, hook string, lefthookDelivers bool) bool { - return lefthookDelivers && fileContains(root, hook, lefthookLauncherMarker) + return lefthookDelivers && isLefthookLauncher(root, hook, hook) +} + +// uncoveredHookPaths names the hooks nothing would run Entire for. +// +// Registering with Lefthook is not enough on its own: git runs a hook only if +// the FILE exists, and Lefthook creates one per hook it knew about when it last +// installed. Entire's config reaches Lefthook through `extends` at run time, so +// a repo whose lefthook.yml declares only pre-commit has no file for the other +// four and nothing dispatches Entire there — while every artifact Entire owns +// is present and correct. Reporting that as delivering is the same false OK +// this integration exists to remove, so the check asks the question git asks: +// is there a file here, and does it lead to Entire? +func uncoveredHookPaths(ctx context.Context) []string { + hooksDir, err := GetHooksDir(ctx) + if err != nil { + return gitHookNames + } + root, err := hooksRootForRemoval(hooksDir) + if err != nil { + return gitHookNames + } + var uncovered []string + for _, hook := range gitHookNames { + // Lefthook's launcher reaches Entire through the integration; Entire's + // own hook reaches it directly. Anything else — absent, a symlink, or + // another tool's script — does not. + if isLefthookLauncher(root, hook, hook) || fileContains(root, hook, entireHookMarker) { + continue + } + uncovered = append(uncovered, hook) + } + return uncovered } // lefthookDeliversHooks reports whether Lefthook will run Entire from its own @@ -758,6 +807,11 @@ func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { return HookDelivery{Manager: LefthookManagerName, Reason: fmt.Sprintf("Could not inspect Entire's Lefthook integration: %v", checkErr)} case current: + if uncovered := uncoveredHookPaths(ctx); len(uncovered) > 0 { + return HookDelivery{Manager: LefthookManagerName, + Reason: fmt.Sprintf("Lefthook has no hook file for %s, so nothing runs Entire there.", + strings.Join(uncovered, ", "))} + } return HookDelivery{OK: true, Manager: LefthookManagerName} } // A declined integration is permanent, not a repair pending: Entire's diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index e9fc07a5bf..4728995b82 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -9,6 +9,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/entireio/cli/cmd/entire/cli/worktreedir" "github.com/stretchr/testify/require" ) @@ -228,7 +229,7 @@ func TestEnsureLefthookIntegration_ReconcilesHookFiles(t *testing.T) { hooksDir := filepath.Join(dir, ".git", "hooks") require.NoError(t, os.MkdirAll(hooksDir, 0o755)) - launcher := []byte("#!/bin/sh\ncall_lefthook run \"pre-push\" \"$@\"\n") + launcher := lefthookLauncher("pre-push") entireHook := []byte("#!/bin/sh\n# " + entireHookMarker + "\nentire hooks git pre-push \"$1\"\n") write := func(name string, body []byte) { require.NoError(t, os.WriteFile(filepath.Join(hooksDir, name), body, 0o755)) @@ -277,21 +278,51 @@ func TestEnsureLefthookIntegration_ReconcilesHookFiles(t *testing.T) { "a hook Lefthook has not taken over is still Entire's to install") } -// Delivery is judged on the integration in a Lefthook repo, because Lefthook -// owns .git/hooks/* and rewrites them constantly. +// Delivery in a Lefthook repo is judged on the integration rather than on the +// contents of .git/hooks/*, which Lefthook owns and rewrites — but it still +// takes a hook FILE to exist, because that is what git runs. Lefthook creates +// one per hook it knew about at its last install, so a repo whose lefthook.yml +// declares only pre-commit has none for the rest, and every artifact Entire +// owns can be present and correct while nothing dispatches Entire at all. func TestCheckHookDelivery_Lefthook(t *testing.T) { - newLefthookRepo(t, "") + dir := newLefthookRepo(t, "") got := CheckHookDelivery(t.Context(), false) require.False(t, got.OK, "not registered yet") - require.Equal(t, "Lefthook", got.Manager) + require.Equal(t, LefthookManagerName, got.Manager) require.NotEmpty(t, got.Reason) _, err := EnsureLefthookIntegration(t.Context(), false) require.NoError(t, err) + + // Registered, but no hook file exists for any hook yet. got = CheckHookDelivery(t.Context(), false) - require.True(t, got.OK) - require.Equal(t, "Lefthook", got.Manager) + require.False(t, got.OK, "a registration nothing triggers is not delivery") + require.Equal(t, LefthookManagerName, got.Manager) + for _, hook := range gitHookNames { + require.Contains(t, got.Reason, hook) + } + + // Lefthook owns one hook; Entire's own hooks cover the rest. Both reach + // Entire, so both count. + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-commit"), lefthookLauncher("pre-commit"), 0o755)) + ClearHooksDirCache() + _, err = ReinstallGitHooks(t.Context()) + require.NoError(t, err) + ClearHooksDirCache() + + got = CheckHookDelivery(t.Context(), false) + require.True(t, got.OK, "reason: %s", got.Reason) + require.Equal(t, LefthookManagerName, got.Manager) require.Empty(t, got.Reason) + + // A hook file that belongs to neither is not delivery either. + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-push"), + []byte("#!/bin/sh\necho someone else\n"), 0o755)) + got = CheckHookDelivery(t.Context(), false) + require.False(t, got.OK) + require.Contains(t, got.Reason, "pre-push") } // In a repo with no hook manager, delivery is the native hook state. @@ -428,8 +459,7 @@ func TestInstallGitHook_ReportsLefthookDeliveryInsteadOfAFalseInstall(t *testing hooksDir := filepath.Join(dir, ".git", "hooks") require.NoError(t, os.MkdirAll(hooksDir, 0o755)) for _, hook := range gitHookNames { - require.NoError(t, os.WriteFile(filepath.Join(hooksDir, hook), - []byte("#!/bin/sh\ncall_lefthook run \""+hook+"\" \"$@\"\n"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, hook), lefthookLauncher(hook), 0o755)) } ClearHooksDirCache() @@ -480,3 +510,47 @@ func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testi require.Empty(t, got.Manager) require.Equal(t, "lefthook-local.toml", got.Declined, "and the reason why is still reported") } + +// lefthookLauncher is shaped like the hook Lefthook 2.1.10 generates: a +// call_lefthook shell function, then a dispatch of this hook through it. +func lefthookLauncher(hook string) []byte { + return []byte("#!/bin/sh\n" + + "if [ \"$LEFTHOOK\" = \"0\" ]; then\n exit 0\nfi\n" + + "call_lefthook()\n{\n lefthook \"$@\"\n}\n\n" + + "call_lefthook run \"" + hook + "\" \"$@\"\n") +} + +// Misreading a hook as Lefthook's is silent and permanent — reconcileHookFiles +// restores it over Entire's and installSkipsHook then skips it forever — so +// recognition is structural, not a search for the word "lefthook". +func TestIsLefthookLauncher(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + root, err := worktreedir.OpenAt(dir) + require.NoError(t, err) + + for _, tc := range []struct { + name string + body []byte + want bool + }{ + {"lefthook's own launcher", lefthookLauncher("pre-push"), true}, + {"a launcher for another hook", lefthookLauncher("pre-commit"), false}, + {"a hand-written hook that runs lefthook", + []byte("#!/bin/sh\n# call_lefthook when staged\nexec lefthook run pre-push \"$@\"\n"), false}, + {"a hook that only names the function", + []byte("#!/bin/sh\ncall_lefthook run \"pre-push\" \"$@\"\n"), false}, + {"Entire's own hook", []byte("#!/bin/sh\n# " + entireHookMarker + "\nentire hooks git pre-push\n"), false}, + {"absent", nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + name := "hook-" + strings.ReplaceAll(tc.name, " ", "-") + if tc.body != nil { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), tc.body, 0o755)) + } + require.Equal(t, tc.want, isLefthookLauncher(root, name, "pre-push")) + }) + } +} diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go new file mode 100644 index 0000000000..521066426f --- /dev/null +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -0,0 +1,131 @@ +package strategy + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestLefthookScriptRunsUnderItsRunnerOnWindows exercises the one assumption +// in the Lefthook integration that does not hold by construction. +// +// Entire's hooks reach Lefthook as `scripts` declared with `runner: bash`, so +// Lefthook resolves bash through PATH and spawns it with the script. Every +// other artifact is a file Entire writes and reads itself, but this step is +// Lefthook executing a POSIX script on a platform whose shell is not POSIX — +// and if bash does not resolve, or the script does not run under it, Entire +// silently stops capturing in every Lefthook repo on Windows. +// +// The name carries "Windows" because that is how ci.yml selects the tests the +// windows-latest job runs. It runs everywhere, which is what makes a failure +// on Windows alone meaningful rather than a missing harness. +// +// Scope worth stating: passing proves bash is resolvable and the script runs +// under it on the machine running the test. A GitHub Windows runner ships Git +// for Windows with bash on PATH; a developer who installed Git with the +// "from cmd only" option may not, and this test cannot speak for that machine. +func TestLefthookScriptRunsUnderItsRunnerOnWindows(t *testing.T) { + t.Parallel() + + bash, err := exec.LookPath("bash") + require.NoErrorf(t, err, "lefthook spawns the declared runner through PATH, "+ + "so no bash on %s means Entire's hooks never run in a Lefthook repo there", runtime.GOOS) + + dir := t.TempDir() + + // A stand-in for the entire binary, so the script's `command -v entire` + // guard is satisfied and the arguments it forwards can be observed. + record := filepath.Join(dir, "args.txt") + stub := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + shellQuote(record) + "\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "entire"), []byte(stub), 0o755)) + + var prePush hookSpec + for _, spec := range buildHookSpecs("entire") { + if spec.name == "pre-push" { + prePush = spec + } + } + require.NotEmpty(t, prePush.name, "pre-push spec") + script := filepath.Join(dir, lefthookScript) + require.NoError(t, os.WriteFile(script, []byte(renderLefthookScript(prePush)), 0o755)) + + // Exactly what Lefthook does: the runner, the script, and the hook's own + // arguments — pre-push gets the remote and its URL. + cmd := exec.CommandContext(t.Context(), bash, lefthookScript, "origin", "https://example.com/x.git") + cmd.Dir = dir + cmd.Env = append(os.Environ(), "PATH="+dir+string(os.PathListSeparator)+os.Getenv("PATH")) + out, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "running Entire's Lefthook script under bash: %s", out) + + got, err := os.ReadFile(record) + require.NoError(t, err, "the script did not reach the entire binary: %s", out) + require.Equal(t, "hooks git pre-push origin", strings.TrimSpace(string(got)), + "the hook's arguments must survive the runner") +} + +// requireLefthookEnv names the variable CI sets so a lefthook that failed to +// install is a failure rather than a silent skip. +const requireLefthookEnv = "ENTIRE_TEST_REQUIRE_LEFTHOOK" + +// TestLefthookDeliversEntireOnWindows is the whole chain with the real +// Lefthook binary: git runs Lefthook's generated hook, Lefthook resolves +// Entire's config through `extends`, and spawns Entire's script with the +// hook's arguments. Nothing here is stubbed except the entire binary itself. +// +// It carries "Windows" in its name so ci.yml's windows-latest job selects it; +// that job installs lefthook and sets ENTIRE_TEST_REQUIRE_LEFTHOOK, so a +// missing binary there fails instead of skipping. Elsewhere it runs whenever +// lefthook happens to be on PATH. +func TestLefthookDeliversEntireOnWindows(t *testing.T) { + lefthook, err := exec.LookPath("lefthook") + if err != nil { + if os.Getenv(requireLefthookEnv) != "" { + t.Fatalf("%s is set but lefthook is not on PATH: %v", requireLefthookEnv, err) + } + t.Skip("lefthook not installed") + } + + dir := newLefthookRepo(t, "") + binDir := filepath.Join(t.TempDir(), "bin") + require.NoError(t, os.MkdirAll(binDir, 0o755)) + record := filepath.Join(binDir, "args.txt") + // hookCmdPrefix resolves to a bare "entire", so a stub on PATH stands in. + require.NoError(t, os.WriteFile(filepath.Join(binDir, "entire"), + []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> "+shellQuote(record)+"\n"), 0o755)) + + env := append(os.Environ(), "PATH="+binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + run := func(name string, args ...string) { + t.Helper() + cmd := exec.CommandContext(t.Context(), name, args...) + cmd.Dir = dir + cmd.Env = env + out, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "%s %v: %s", name, args, out) + } + + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("hi\n"), 0o644)) + run("git", "add", "a.txt") + run("git", "commit", "-m", "init", "--no-verify") + + _, err = EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + // Lefthook only generates a hook file per hook it knows about, and it + // learns Entire's from the extends entry written above. + run(lefthook, "install") + + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("hi\nthere\n"), 0o644)) + run("git", "add", "a.txt") + run("git", "commit", "-m", "captured") + + got, err := os.ReadFile(record) + require.NoError(t, err, "Lefthook never reached Entire") + lines := strings.Fields(strings.ReplaceAll(strings.TrimSpace(string(got)), "\n", " ")) + for _, hook := range []string{"prepare-commit-msg", "commit-msg", "post-commit"} { + require.Containsf(t, lines, hook, "Lefthook did not dispatch %s to Entire: %q", hook, got) + } +} From 3aa93ffab6abcd7a692267f16fe25bc7ea4f22c6 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 17:07:26 -0700 Subject: [PATCH 05/18] test(hooks): close memoized roots so Windows can clean up its temp repos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows job's first run showed the Lefthook integration working there — bash resolved, the script ran with the hook's arguments, and every assertion in the lefthook-driven end-to-end test passed — but the test still failed, in t.TempDir's cleanup: unlinkat ...\.git: The process cannot access the file because it is being used by another process osroot.Shared memoizes a *os.Root per directory, so the worktree root and git common dir this test opened stay open for the life of the process, and an open handle stops Windows deleting the directory under it. The helper now calls osroot.ResetShared alongside the two path caches it already cleared; being registered after t.TempDir, LIFO runs it first. Also ToSlash the recorder path interpolated into the stub's shell script: bash keeps backslashes literal inside single quotes, so a Windows path would have become a mangled filename rather than a directory walk. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29F1G8DD3NWCDJ44WFBP13R --- cmd/entire/cli/strategy/lefthook_test.go | 15 ++++++++++----- .../cli/strategy/lefthook_windows_runner_test.go | 7 +++++-- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 4728995b82..cbcea207b2 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/entireio/cli/cmd/entire/cli/worktreedir" @@ -27,14 +28,18 @@ func newLefthookRepo(t *testing.T, mainConfig string) string { // Both caches are process-global and keyed by the directory this test just // left, so a stale entry would point the hooks-dir and common-dir lookups // at another test's repository. - paths.ClearWorktreeRootCache() - ClearHooksDirCache() - clearGitCommonDirCache() - t.Cleanup(func() { + reset := func() { paths.ClearWorktreeRootCache() ClearHooksDirCache() clearGitCommonDirCache() - }) + // osroot.Shared memoizes a *os.Root per directory, and an open handle + // keeps Windows from deleting the directory under it — t.TempDir's + // own cleanup then fails with "used by another process" on .git. + // Registered after t.TempDir, so LIFO runs this first. + osroot.ResetShared() + } + reset() + t.Cleanup(reset) return dir } diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go index 521066426f..f0e511d885 100644 --- a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -40,8 +40,11 @@ func TestLefthookScriptRunsUnderItsRunnerOnWindows(t *testing.T) { // A stand-in for the entire binary, so the script's `command -v entire` // guard is satisfied and the arguments it forwards can be observed. + // ToSlash because the path is interpolated into a shell script: bash keeps + // backslashes literal inside single quotes, so a Windows path would be a + // mangled filename rather than a directory walk. record := filepath.Join(dir, "args.txt") - stub := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + shellQuote(record) + "\n" + stub := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + shellQuote(filepath.ToSlash(record)) + "\n" require.NoError(t, os.WriteFile(filepath.Join(dir, "entire"), []byte(stub), 0o755)) var prePush hookSpec @@ -96,7 +99,7 @@ func TestLefthookDeliversEntireOnWindows(t *testing.T) { record := filepath.Join(binDir, "args.txt") // hookCmdPrefix resolves to a bare "entire", so a stub on PATH stands in. require.NoError(t, os.WriteFile(filepath.Join(binDir, "entire"), - []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> "+shellQuote(record)+"\n"), 0o755)) + []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> "+shellQuote(filepath.ToSlash(record))+"\n"), 0o755)) env := append(os.Environ(), "PATH="+binDir+string(os.PathListSeparator)+os.Getenv("PATH")) run := func(name string, args ...string) { From 582495f2c8f833cef1f3a16c9f0364c25356254b Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 17:42:55 -0700 Subject: [PATCH 06/18] fix(hooks): decline a tracked Lefthook local config, and stop claiming no action needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both about the paths where Entire declines to register. hookManagerWarning printed "No action needed" for every Lefthook repo, without checking whether Entire could actually register. Where it has declined, the repo runs on native hooks that Lefthook reclaims — so the message was the same false advice (#2263) this integration set out to remove, reintroduced in the branch that replaced it. The Lefthook case now splits: a Note with "No action needed" only when registration is possible, and otherwise a Warning naming the reason and saying plainly that Lefthook reclaims the hooks it manages, Entire reinstalls them on the next agent turn, and a commit made in between is not captured. ensureExtendsEntry wrote into whatever lefthook-local.* it found, with no check for whether the repository carries that file. Lefthook documents it as personal and gitignored, but a committed one is the team's file: adding Entire's entry modifies something shared, and a developer who reverts it gets it back on the next turn. Entire now declines (ErrLefthookLocalConfigTracked), reusing the Declined channel that already reports the non-YAML refusal. The check runs only where the entry would actually be ADDED, so a tracked config that already carries it — committed by the team, or added by hand — is honoured and costs nothing after the first install. One correction to the finding: the entry was written once, not on every turn; ensureExtendsEntry is idempotent and only runs when the currency check fails. The tracked probe is settings.PathIsTracked rather than a second index scan. probePathIsVersioned took a path but used it only to locate the repo, comparing against the EntireSettingsLocalFile constant — a single-purpose helper whose signature looked general — so it now takes the repo root and a repo-relative name, and the Dir(Dir(path)) inference moved to the settings callers whose path shape justifies it. Duplicating it would have been worse than untidy: a153514f8 and 6c4adc454 exist because an exact comparison misses a tracked file on a case-insensitive volume, and the open-coded copy this replaces had reintroduced exactly that bug. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29H2FBXXRQ51MV5028Q0MDK --- cmd/entire/cli/doctor.go | 4 +- cmd/entire/cli/settings/opf_command_trust.go | 46 ++++++++++++---- .../cli/settings/opf_command_trust_test.go | 20 +++---- cmd/entire/cli/strategy/common.go | 4 +- cmd/entire/cli/strategy/hook_managers.go | 20 ++++++- cmd/entire/cli/strategy/hook_managers_test.go | 38 ++++++++++--- cmd/entire/cli/strategy/lefthook.go | 55 ++++++++++++++++--- cmd/entire/cli/strategy/lefthook_test.go | 43 ++++++++++++++- 8 files changed, 184 insertions(+), 46 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 6a4b725f34..fe381526d9 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -642,8 +642,8 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { // Entire's own hooks are the arrangement here, so the checks below are // the right ones — but say why Lefthook is not carrying it, since // otherwise a Lefthook repo silently looks like any other. - fmt.Fprintf(w, "Note: Entire is not registered in Lefthook's config (%s is not YAML,\n", delivery.Declined) - fmt.Fprintln(w, " and Entire will not shadow it). Entire's own Git hooks are used instead.") + fmt.Fprintf(w, "Note: Entire is not registered in Lefthook's config (%s,\n", delivery.Declined) + fmt.Fprintln(w, " and Entire will not modify it). Entire's own Git hooks are used instead.") } if delivery.Manager == strategy.LefthookManagerName { if delivery.OK { diff --git a/cmd/entire/cli/settings/opf_command_trust.go b/cmd/entire/cli/settings/opf_command_trust.go index 03a53580e0..4284d1a461 100644 --- a/cmd/entire/cli/settings/opf_command_trust.go +++ b/cmd/entire/cli/settings/opf_command_trust.go @@ -158,6 +158,24 @@ const ( localOwn // verified not versioned, or no repository to clone from ) +// PathIsTracked reports whether a repo-relative path is carried by the +// repository — present in the index of any clone that checks it out. +// +// Exported so callers outside this package do not hand-roll the probe: two +// commits' worth of correctness lives in it that an open-coded index scan +// does not get. `pathsEqualFold` rather than an exact compare, because on a +// case-insensitive volume a differently-cased path is the same file; a +// missing repository is a definitive "not tracked" rather than a failure; +// an error means "could not determine" and must not be read as a negative; +// and the answer is memoized, since trackedness cannot change inside one +// short-lived hook process. +// +// Index-only, matching classifyLocalSettings: the question is whether the +// repository carries the file, and HEAD costs more than it answers here. +func PathIsTracked(ctx context.Context, repoRoot, relPath string) (bool, error) { + return pathIsVersioned(ctx, repoRoot, relPath, false) +} + // classifyLocalSettings checks the git index only. // // The index is what a delivered attack shows up in: a pull request that @@ -185,7 +203,10 @@ func classifyLocalSettingsDeep(ctx context.Context, path string) localTrust { } func classify(ctx context.Context, path string, deep bool) localTrust { - switch versioned, err := localSettingsIsVersioned(ctx, path, deep); { + // The settings file is always /.entire/settings.local.json, so + // its root is two levels up. Callers that know their root pass it + // directly through PathIsTracked. + switch versioned, err := pathIsVersioned(ctx, filepath.Dir(filepath.Dir(path)), EntireSettingsLocalFile, deep); { case err != nil: return localUnverifiable case versioned: @@ -199,7 +220,8 @@ func classify(ctx context.Context, path string, deep bool) localTrust { // subsumes the shallow one, but they are asked by different callers and only // the rare OPF path pays for HEAD. type probeKey struct { - path string + root string + rel string deep bool } @@ -230,13 +252,13 @@ func ClearVersionedPathCache() { clear(versionedPaths) } -// localSettingsIsVersioned reports whether the local settings file is tracked -// in the index, and with deep set, whether it is also present in HEAD. +// pathIsVersioned reports whether a repo-relative path is tracked in the +// index, and with deep set, whether it is also present in HEAD. // // An error means "could not determine" (unreadable repository, unreadable // index) and callers must treat it as untrusted rather than as a negative. -func localSettingsIsVersioned(ctx context.Context, path string, deep bool) (bool, error) { - key := probeKey{path: path, deep: deep} +func pathIsVersioned(ctx context.Context, repoRoot, rel string, deep bool) (bool, error) { + key := probeKey{root: repoRoot, rel: rel, deep: deep} versionedPathsMu.Lock() cached, ok := versionedPaths[key] @@ -245,7 +267,7 @@ func localSettingsIsVersioned(ctx context.Context, path string, deep bool) (bool return cached, nil } - versioned, err := probeLocalSettingsIsVersioned(ctx, path, deep) + versioned, err := probePathIsVersioned(ctx, repoRoot, rel, deep) if err != nil { return false, err } @@ -271,12 +293,12 @@ func localSettingsIsVersioned(ctx context.Context, path string, deep bool) (bool // the grandparent would name the wrong directory. It holds today because // readConfined can only have succeeded on the relative form when the working // directory IS the worktree root. -func probeLocalSettingsIsVersioned(ctx context.Context, path string, deep bool) (bool, error) { +func probePathIsVersioned(ctx context.Context, repoRoot, rel string, deep bool) (bool, error) { if err := ctx.Err(); err != nil { - return false, fmt.Errorf("verify %s: %w", EntireSettingsLocalFile, err) + return false, fmt.Errorf("verify %s: %w", rel, err) } - repo, err := gitrepo.OpenPath(filepath.Dir(filepath.Dir(path))) + repo, err := gitrepo.OpenPath(repoRoot) if err != nil { // No repository at all (no .git). A file cannot have arrived by // cloning when there is nothing to clone, so this is a definitive @@ -296,7 +318,7 @@ func probeLocalSettingsIsVersioned(ctx context.Context, path string, deep bool) return false, fmt.Errorf("read index: %w", err) } for _, entry := range idx.Entries { - if pathsEqualFold(entry.Name, EntireSettingsLocalFile) { + if pathsEqualFold(entry.Name, rel) { return true, nil } } @@ -321,7 +343,7 @@ func probeLocalSettingsIsVersioned(ctx context.Context, path string, deep bool) if err != nil { return false, fmt.Errorf("read HEAD tree: %w", err) } - return treeHasPathFold(tree, strings.Split(EntireSettingsLocalFile, "/")) + return treeHasPathFold(tree, strings.Split(rel, "/")) } // pathsEqualFold reports whether two repo-relative git paths can name the same diff --git a/cmd/entire/cli/settings/opf_command_trust_test.go b/cmd/entire/cli/settings/opf_command_trust_test.go index e079727630..7f1975c8db 100644 --- a/cmd/entire/cli/settings/opf_command_trust_test.go +++ b/cmd/entire/cli/settings/opf_command_trust_test.go @@ -234,18 +234,18 @@ func TestPathIsVersioned_MemoizesWithinProcess(t *testing.T) { root, _, local := newOPFRepo(t) writeSettingsFile(t, local, localOPFSettings(attackerCommand)) - first, err := localSettingsIsVersioned(t.Context(), local, true) + first, err := pathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err) require.False(t, first, "file is untracked to begin with") testutil.RunGit(t, root, "add", "-f", EntireSettingsLocalFile) - second, err := localSettingsIsVersioned(t.Context(), local, true) + second, err := pathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err) assert.False(t, second, "result is cached for the process lifetime") ClearVersionedPathCache() - third, err := localSettingsIsVersioned(t.Context(), local, true) + third, err := pathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err) assert.True(t, third, "the reset seam must drop the cached verdict") } @@ -263,7 +263,7 @@ func TestPathIsVersioned_NoGitBinaryRequired(t *testing.T) { t.Setenv("PATH", "") - versioned, err := probeLocalSettingsIsVersioned(t.Context(), local, true) + versioned, err := probePathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err, "probe must not depend on the git binary") assert.True(t, versioned, "committed file must still be detected") } @@ -284,12 +284,12 @@ func TestPathIsVersioned_LinkedWorktreeUsesOwnIndex(t *testing.T) { local := filepath.Join(wt, EntireSettingsLocalFile) require.NoError(t, os.WriteFile(local, []byte(localOPFSettings(trustedCommand)), 0o644)) - versioned, err := probeLocalSettingsIsVersioned(t.Context(), local, true) + versioned, err := probePathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err) assert.False(t, versioned, "untracked in the linked worktree") testutil.RunGit(t, wt, "add", "-f", EntireSettingsLocalFile) - versioned, err = probeLocalSettingsIsVersioned(t.Context(), local, true) + versioned, err = probePathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err) assert.True(t, versioned, "must read the linked worktree's own index") } @@ -316,7 +316,7 @@ func TestPathIsVersioned_ReftableRepo(t *testing.T) { testutil.RunGit(t, root, "commit", "-m", "carry") testutil.RunGit(t, root, "rm", "--cached", EntireSettingsLocalFile) - versioned, err := probeLocalSettingsIsVersioned(t.Context(), local, true) + versioned, err := probePathIsVersioned(t.Context(), filepath.Dir(filepath.Dir(local)), EntireSettingsLocalFile, true) require.NoError(t, err, "reftable repo must be verifiable") assert.True(t, versioned, "HEAD lookup must work on reftable") } @@ -528,8 +528,7 @@ func TestPathIsVersioned_DecoySiblingDoesNotMaskRealEntry(t *testing.T) { commitIndexAndClear(t, root, tc.decoy, EntireSettingsLocalFile) ClearVersionedPathCache() - got, err := probeLocalSettingsIsVersioned( - t.Context(), filepath.Join(root, EntireSettingsLocalFile), true) + got, err := probePathIsVersioned(t.Context(), root, EntireSettingsLocalFile, true) require.NoError(t, err) assert.True(t, got, "the real entry must be found past the earlier-sorting decoy") }) @@ -554,8 +553,7 @@ func TestPathIsVersioned_Win32TrailingCharVariantsAreTracked(t *testing.T) { stageBlobAt(t, root, variant) ClearVersionedPathCache() - got, err := probeLocalSettingsIsVersioned( - t.Context(), filepath.Join(root, EntireSettingsLocalFile), false) + got, err := probePathIsVersioned(t.Context(), root, EntireSettingsLocalFile, false) require.NoError(t, err) assert.True(t, got, "a Win32-equivalent committed name must count as tracked") }) diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index a3dfca9b3c..e320862c09 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -1840,7 +1840,9 @@ func ensureLefthookIntegrationIfManaged(ctx context.Context) error { return reconcileHookFiles(ctx) } if _, err := EnsureLefthookIntegration(ctx, absolute); err != nil { - if errors.Is(err, ErrLefthookLocalConfigUnwritable) { + // Both refusals are decisions about the user's repository, not + // failures: the native hooks stay and status/doctor report why. + if errors.Is(err, ErrLefthookLocalConfigUnwritable) || errors.Is(err, ErrLefthookLocalConfigTracked) { return nil } return fmt.Errorf("failed to register Entire with Lefthook: %w", err) diff --git a/cmd/entire/cli/strategy/hook_managers.go b/cmd/entire/cli/strategy/hook_managers.go index 8f8849c3f6..dd08bbfa44 100644 --- a/cmd/entire/cli/strategy/hook_managers.go +++ b/cmd/entire/cli/strategy/hook_managers.go @@ -67,8 +67,10 @@ func detectHookManagers(repoRoot string) []hookManager { } // hookManagerWarning builds a warning string for detected hook managers. -// cmdPrefix is the CLI command prefix (e.g., "entire" or an absolute binary path). -func hookManagerWarning(managers []hookManager, cmdPrefix string) string { +// cmdPrefix is the CLI command prefix (e.g., "entire" or an absolute binary +// path); declined is the phrase from declinedLefthookLocalConfig, empty when +// Entire is able to register with Lefthook. +func hookManagerWarning(managers []hookManager, cmdPrefix, declined string) string { if len(managers) == 0 { return "" } @@ -78,6 +80,18 @@ func hookManagerWarning(managers []hookManager, cmdPrefix string) string { for _, m := range managers { switch m.Name { case LefthookManagerName: + if declined != "" { + // "No action needed" is true only when Entire can register. + // Where it has declined, the repo is on native hooks that + // Lefthook reclaims — telling the user otherwise is the false + // advice this integration was meant to remove (#2263). + fmt.Fprintf(&b, "Warning: %s detected (%s)\n\n", m.Name, m.ConfigPath) + fmt.Fprintf(&b, " Entire could not register in %s's configuration: %s,\n", m.Name, declined) + fmt.Fprintf(&b, " and Entire will not modify it. Entire's own hooks are used instead,\n") + fmt.Fprintf(&b, " and %s reclaims the hooks it manages — Entire reinstalls them on\n", m.Name) + fmt.Fprintf(&b, " the next agent turn, so a commit made in between is not captured.\n\n") + break + } // Entire registers itself in Lefthook's own config, so Lefthook's // regenerations no longer remove it and there is nothing for the // user to do. See EnsureLefthookIntegration. @@ -143,7 +157,7 @@ func CheckAndWarnHookManagers(ctx context.Context, w io.Writer, absolutePath boo // Best-effort: hook manager warnings are advisory, skip on resolution failure return } - warning := hookManagerWarning(managers, cmdPrefix) + warning := hookManagerWarning(managers, cmdPrefix, declinedLefthookLocalConfig(ctx, repoRoot)) if warning != "" { fmt.Fprintln(w) fmt.Fprint(w, warning) diff --git a/cmd/entire/cli/strategy/hook_managers_test.go b/cmd/entire/cli/strategy/hook_managers_test.go index 9391a9c31a..5f53421639 100644 --- a/cmd/entire/cli/strategy/hook_managers_test.go +++ b/cmd/entire/cli/strategy/hook_managers_test.go @@ -302,7 +302,7 @@ func TestHookManagerWarning_Husky(t *testing.T) { {Name: "Husky", ConfigPath: ".husky/"}, } - warning := hookManagerWarning(managers, "entire") + warning := hookManagerWarning(managers, "entire", "") // Should contain all 4 hook file references for _, hook := range gitHookNames { @@ -340,7 +340,7 @@ func TestHookManagerWarning_Lefthook(t *testing.T) { {Name: "Lefthook", ConfigPath: "lefthook.yml"}, } - warning := hookManagerWarning(managers, "entire") + warning := hookManagerWarning(managers, "entire", "") // Lefthook is the one manager Entire integrates with at the config level, // so the message must not ask the user to do anything. @@ -367,7 +367,7 @@ func TestHookManagerWarning_OverwritesAtInstall(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - warning := hookManagerWarning([]hookManager{{Name: name, ConfigPath: "cfg"}}, "entire") + warning := hookManagerWarning([]hookManager{{Name: name, ConfigPath: "cfg"}}, "entire", "") if !strings.Contains(warning, "Warning: "+name+" detected") { t.Errorf("warning should name %s, got %q", name, warning) @@ -385,12 +385,12 @@ func TestHookManagerWarning_OverwritesAtInstall(t *testing.T) { func TestHookManagerWarning_Empty(t *testing.T) { t.Parallel() - warning := hookManagerWarning(nil, "entire") + warning := hookManagerWarning(nil, "entire", "") if warning != "" { t.Errorf("expected empty string for nil managers, got %q", warning) } - warning = hookManagerWarning([]hookManager{}, "entire") + warning = hookManagerWarning([]hookManager{}, "entire", "") if warning != "" { t.Errorf("expected empty string for empty managers, got %q", warning) } @@ -408,7 +408,7 @@ func TestHookManagerWarning_AbsolutePathPrefix(t *testing.T) { // warning must quote it verbatim so the command it tells the user to add is // the one Entire actually installs. const prefix = "'/opt/homebrew/bin/entire'" - warning := hookManagerWarning(managers, prefix) + warning := hookManagerWarning(managers, prefix, "") if !strings.Contains(warning, prefix+" hooks git") { t.Errorf("warning should use the resolved command prefix, got %q", warning) @@ -423,7 +423,7 @@ func TestHookManagerWarning_Multiple(t *testing.T) { {Name: "Lefthook", ConfigPath: "lefthook.yml"}, } - warning := hookManagerWarning(managers, "entire") + warning := hookManagerWarning(managers, "entire", "") if !strings.Contains(warning, "Warning: Husky detected") { t.Error("should contain Husky warning") @@ -508,3 +508,27 @@ func TestCheckAndWarnHookManagers_WithHusky(t *testing.T) { t.Errorf("expected warning output, got %q", output) } } + +// "No action needed" is true only where Entire can register. Where it has +// declined, the repo runs on native hooks that Lefthook reclaims, and saying +// otherwise is the false advice this integration set out to remove. +func TestHookManagerWarning_LefthookDeclined(t *testing.T) { + t.Parallel() + + managers := []hookManager{{Name: LefthookManagerName, ConfigPath: "lefthook.yml"}} + warning := hookManagerWarning(managers, "entire", "lefthook-local.toml is not YAML") + + if strings.Contains(warning, "No action needed") { + t.Errorf("a declined registration must not claim there is nothing to do, got:\n%s", warning) + } + for _, want := range []string{ + "Warning: Lefthook detected", + "lefthook-local.toml is not YAML", + "reclaims the hooks it manages", + "not captured", + } { + if !strings.Contains(warning, want) { + t.Errorf("warning should explain %q, got:\n%s", want, warning) + } + } +} diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 92eaa4af8f..4afb102118 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -33,6 +33,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/jsonutil" "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/worktreedir" "gopkg.in/yaml.v3" ) @@ -65,6 +66,13 @@ var ErrLefthookLocalConfigUnwritable = errors.New("lefthook local config is not // to touch the repo and is handled silently. var ErrLefthookArtifactBlocked = errors.New("lefthook artifact path is blocked by an existing backup") +// ErrLefthookLocalConfigTracked reports a local config the repository carries. +// Lefthook documents lefthook-local.* as personal and gitignored; a tracked +// one is the team's file, so Entire declines to add its entry rather than +// modify something shared. Like ErrLefthookLocalConfigUnwritable this is a +// decision, not a failure, and the native hooks stay. +var ErrLefthookLocalConfigTracked = errors.New("lefthook local config is tracked by git") + // lefthookLocalConfigNames are the local config names Lefthook reads, in its // own precedence order. Writing into anything but the first match present // would be a silent no-op. @@ -122,8 +130,13 @@ func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, err // the point of excluding — and because ensureLefthookIntegrationIfManaged // treats this refusal as a decision rather than a failure, that recurred // on every turn instead of resolving itself. - if _, _, err := findLocalConfig(root); err != nil { + if name, existing, err := findLocalConfig(root); err != nil { return 0, err + } else if existing != nil && !extendsEntryPresent(root) && lefthookLocalConfigTracked(ctx, repoRoot, name) { + // Only when the entry would actually be ADDED: a tracked config that + // already carries it (committed by the team, or added by hand) is + // working as intended, and nothing here writes to it. + return 0, fmt.Errorf("%w: %s", ErrLefthookLocalConfigTracked, name) } cmdPrefix, err := hookCmdPrefix(absolutePath) if err != nil { @@ -818,7 +831,7 @@ func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { // own hooks are the arrangement in such a repo, so the answer is the // native one. Saying "not registered with Lefthook" here would report // a working repository as broken, forever and with no fix to offer. - if declined = declinedLefthookLocalConfig(repoRoot); declined == "" { + if declined = declinedLefthookLocalConfig(ctx, repoRoot); declined == "" { return HookDelivery{Manager: LefthookManagerName, Reason: "Entire is not registered with Lefthook, so Lefthook's hooks do not run it."} } @@ -833,16 +846,42 @@ func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { return HookDelivery{Reason: "Entire's Git hooks are not installed.", Declined: declined} } -// declinedLefthookLocalConfig names the local config Entire refuses to write -// to, or "" when there is none to refuse. -func declinedLefthookLocalConfig(repoRoot string) string { +// declinedLefthookLocalConfig describes the local config Entire refuses to +// write to, as a phrase naming the file and why — or "" when nothing is being +// refused. A phrase rather than a name because the two reasons need different +// remedies and every consumer of HookDelivery.Declined is reporting it to a +// person. +func declinedLefthookLocalConfig(ctx context.Context, repoRoot string) string { root, err := worktreedir.OpenAt(repoRoot) if err != nil { return "" } - name, _, err := findLocalConfig(root) - if errors.Is(err, ErrLefthookLocalConfigUnwritable) { - return name + name, existing, err := findLocalConfig(root) + switch { + case errors.Is(err, ErrLefthookLocalConfigUnwritable): + return name + " is not YAML" + case err != nil: + return "" + case existing != nil && !extendsEntryPresent(root) && lefthookLocalConfigTracked(ctx, repoRoot, name): + return name + " is tracked by git" } return "" } + +// lefthookLocalConfigTracked reports whether the local config is carried by +// the repository rather than being the per-clone file Lefthook documents. +// +// A tracked one is shared with the team, so adding Entire's extends entry to +// it modifies their file, not the developer's — and a revert followed by a +// reinstall next turn is a fight Entire should not pick. +// +// settings.PathIsTracked rather than an index scan here: that probe carries +// two commits' worth of correctness an open-coded one does not, in particular +// comparing paths the way the filesystem would, because on a case-insensitive +// volume a differently-cased tracked path is the same file. It is consulted +// only when Entire is about to add the entry, so after a successful install it +// never runs again. +func lefthookLocalConfigTracked(ctx context.Context, repoRoot, name string) bool { + tracked, err := settings.PathIsTracked(ctx, repoRoot, name) + return err == nil && tracked +} diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index cbcea207b2..9b9dd1f5fa 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -9,6 +9,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/entireio/cli/cmd/entire/cli/worktreedir" "github.com/stretchr/testify/require" @@ -32,6 +33,9 @@ func newLefthookRepo(t *testing.T, mainConfig string) string { paths.ClearWorktreeRootCache() ClearHooksDirCache() clearGitCommonDirCache() + // PathIsTracked memoizes per (root, path) for the process, and these + // tests change a file's tracked state inside one. + settings.ClearVersionedPathCache() // osroot.Shared memoizes a *os.Root per directory, and an open handle // keeps Windows from deleting the directory under it — t.TempDir's // own cleanup then fails with "used by another process" on .git. @@ -503,7 +507,7 @@ func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testi got := CheckHookDelivery(t.Context(), false) require.False(t, got.OK, "no hooks installed yet") require.Empty(t, got.Manager, "Lefthook is not the one delivering") - require.Equal(t, "lefthook-local.toml", got.Declined) + require.Equal(t, "lefthook-local.toml is not YAML", got.Declined) require.Contains(t, got.Reason, "not installed", "the reason must be the native one") _, err := ReinstallGitHooks(t.Context()) @@ -513,7 +517,7 @@ func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testi got = CheckHookDelivery(t.Context(), false) require.True(t, got.OK, "Entire's own hooks deliver here") require.Empty(t, got.Manager) - require.Equal(t, "lefthook-local.toml", got.Declined, "and the reason why is still reported") + require.Equal(t, "lefthook-local.toml is not YAML", got.Declined, "and the reason why is still reported") } // lefthookLauncher is shaped like the hook Lefthook 2.1.10 generates: a @@ -559,3 +563,38 @@ func TestIsLefthookLauncher(t *testing.T) { }) } } + +// Lefthook documents lefthook-local.* as personal and gitignored. A tracked +// one is the team's file: adding Entire's entry modifies something shared, and +// a developer who reverts it gets it back next turn. Entire declines instead — +// but only when it would actually add the entry, so a config that already +// carries it (committed, or added by hand) is honoured. +func TestEnsureLefthookIntegration_TrackedLocalConfig(t *testing.T) { + dir := newLefthookRepo(t, "") + localPath := filepath.Join(dir, "lefthook-local.yml") + original := []byte("pre-commit:\n commands:\n ours:\n run: true\n") + require.NoError(t, os.WriteFile(localPath, original, 0o644)) + testutil.GitAdd(t, dir, "lefthook-local.yml") + testutil.GitCommit(t, dir, "team config") + + _, err := EnsureLefthookIntegration(t.Context(), false) + require.ErrorIs(t, err, ErrLefthookLocalConfigTracked) + + after, err := os.ReadFile(localPath) + require.NoError(t, err) + require.Equal(t, original, after, "a tracked config must not be modified") + + got := CheckHookDelivery(t.Context(), false) + require.Empty(t, got.Manager, "Lefthook is not the one delivering") + require.Equal(t, "lefthook-local.yml is tracked by git", got.Declined) + + // Once the entry is there — committed by the team, or added by hand — + // Entire has nothing to write and the integration proceeds. + require.NoError(t, os.WriteFile(localPath, + append([]byte("extends:\n - "+entireLefthookConfig+"\n"), original...), 0o644)) + _, err = EnsureLefthookIntegration(t.Context(), false) + require.NoError(t, err) + current, err := LefthookIntegrationCurrent(t.Context(), false) + require.NoError(t, err) + require.True(t, current) +} From 3bffe2c4bf701ffc28bdb8dd739f9c011227fd7c Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 11 Sep 2026 17:49:26 -0700 Subject: [PATCH 07/18] fix(hooks): read the hook prefix from settings, not from the caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `entire doctor` called CheckHookDelivery and EnsureLefthookIntegration with a hardcoded absolutePath=false while status passed the setting and EnsureSetup resolved it. In a repo configured with absolute_git_hook_path, doctor therefore compared the installed scripts against the bare-command rendering, reported a working install as NOT DELIVERING, and under --force rewrote the scripts and config down to a bare `entire` — silently breaking delivery for the GUI git clients that setting exists to serve. Fixing doctor alone would leave the footgun: this is exactly what ReinstallGitHooks was introduced to prevent ("so callers outside this package cannot open-code it and silently drop absolute_git_hook_path"), and the new exported functions had reintroduced it. They now resolve the prefix from the repository's settings themselves, and the parameter is gone, so no caller can choose it wrongly. InstallGitHook keeps its own flag; only the question "what does this repository's installed integration say" is resolved internally. TestLefthookIntegration_HonoursAbsoluteGitHookPath pins it: an absolute-path repo installs the absolute form, reads as current rather than needing repair, and is byte-identical after a second install. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M29HECQX4J0E382N3R8HZBZ6 --- cmd/entire/cli/doctor.go | 6 +- cmd/entire/cli/status.go | 8 +- cmd/entire/cli/status_test.go | 2 +- cmd/entire/cli/strategy/common.go | 5 +- cmd/entire/cli/strategy/hooks.go | 4 +- cmd/entire/cli/strategy/lefthook.go | 24 +++-- cmd/entire/cli/strategy/lefthook_test.go | 97 +++++++++++++------ .../strategy/lefthook_windows_runner_test.go | 2 +- 8 files changed, 96 insertions(+), 52 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index fe381526d9..5ee83dd7a7 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -637,7 +637,7 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { // their contents say nothing about whether Entire runs. Every other // manager only overwrites at install time, so for those the hook files // remain the answer and the check below is the right one. - delivery := strategy.CheckHookDelivery(ctx, false) + delivery := strategy.CheckHookDelivery(ctx) if delivery.Declined != "" { // Entire's own hooks are the arrangement here, so the checks below are // the right ones — but say why Lefthook is not carrying it, since @@ -652,7 +652,7 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { // launcher, so every hook runs Entire twice — gets straightened // out. Delivery reads OK in that state, so it needs asking for. if force { - if _, err := strategy.EnsureLefthookIntegration(ctx, false); err != nil { + if _, err := strategy.EnsureLefthookIntegration(ctx); err != nil { return fmt.Errorf("reconcile %s hooks: %w", delivery.Manager, err) } } @@ -666,7 +666,7 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { fmt.Fprintln(w, " Run `entire doctor --force` to apply it.") return nil } - if _, err := strategy.EnsureLefthookIntegration(ctx, false); err != nil { + if _, err := strategy.EnsureLefthookIntegration(ctx); err != nil { return fmt.Errorf("register Entire with Lefthook: %w", err) } // Registering is not enough when Lefthook has no hook file for a hook diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index aa510a229e..d1f6f4b81f 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -247,7 +247,7 @@ func formatSettingsStatusShort(ctx context.Context, s *EntireSettings, sty statu // Where checkpoint data syncs (the single elected remote), and how many // checkpoints have not reached it yet. Local-only computation. if s.Enabled { - writeHookDeliveryLine(ctx, &b, s, sty) + writeHookDeliveryLine(ctx, &b, sty) writeCheckpointSyncLines(ctx, &b, s, sty) } @@ -569,8 +569,8 @@ func countUnpushedCheckpointsForStatus(ctx context.Context, remoteName string) i // reach it, which is issue #2264. The destination line below is still shown — // it is true, and status is the only place it appears — but this line above it // removes the claim that delivery is working. -func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, s *EntireSettings, sty statusStyles) { - delivery := strategy.CheckHookDelivery(ctx, s.AbsoluteGitHookPath) +func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusStyles) { + delivery := strategy.CheckHookDelivery(ctx) b.WriteString("\n") switch { case delivery.OK && delivery.Manager != "": @@ -1202,7 +1202,7 @@ func runStatusJSON(ctx context.Context, w io.Writer) error { result.CheckpointSyncRemote = syncInfo.Remote result.CheckpointSyncRemoteSource = syncInfo.Source result.CheckpointSyncError = syncInfo.Err - delivery := strategy.CheckHookDelivery(ctx, s.AbsoluteGitHookPath) + delivery := strategy.CheckHookDelivery(ctx) result.HooksDeliver = delivery.OK result.HooksManager = delivery.Manager result.HooksDeliverReason = delivery.Reason diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 03a844efdb..2d44639b02 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -3310,7 +3310,7 @@ func TestRunStatus_HookDeliveryLine(t *testing.T) { t.Errorf("unregistered Lefthook repo must warn, got:\n%s", out) } - if _, err := strategy.EnsureLefthookIntegration(context.Background(), false); err != nil { + if _, err := strategy.EnsureLefthookIntegration(context.Background()); err != nil { t.Fatalf("EnsureLefthookIntegration() error = %v", err) } // Registering is not delivery on its own: git runs a hook only if the file diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index e320862c09..1dc16f689c 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -1831,15 +1831,14 @@ func ensureLefthookIntegrationIfManaged(ctx context.Context) error { if err != nil || !LefthookManaged(repoRoot) { return nil //nolint:nilerr // not a Lefthook repo: nothing to do } - absolute := hookSettingsFromConfig(ctx) - current, err := LefthookIntegrationCurrent(ctx, absolute) + current, err := LefthookIntegrationCurrent(ctx) if err != nil { return nil //nolint:nilerr // inspection failure falls back to native hooks } if current { return reconcileHookFiles(ctx) } - if _, err := EnsureLefthookIntegration(ctx, absolute); err != nil { + if _, err := EnsureLefthookIntegration(ctx); err != nil { // Both refusals are decisions about the user's repository, not // failures: the native hooks stay and status/doctor report why. if errors.Is(err, ErrLefthookLocalConfigUnwritable) || errors.Is(err, ErrLefthookLocalConfigTracked) { diff --git a/cmd/entire/cli/strategy/hooks.go b/cmd/entire/cli/strategy/hooks.go index 0b3cf75997..854fa2f57e 100644 --- a/cmd/entire/cli/strategy/hooks.go +++ b/cmd/entire/cli/strategy/hooks.go @@ -715,7 +715,9 @@ func InstallGitHook(ctx context.Context, silent, absolutePath bool) (int, error) installedCount := 0 // In a Lefthook repo the hooks Lefthook owns already run Entire from its // own config; see installSkipsHook. - lefthookDelivers := lefthookDeliversHooks(ctx, absolutePath) + // Resolved from settings, not from this call's absolutePath: the question + // is what the repository's own installed integration says. + lefthookDelivers := lefthookDeliversHooks(ctx) skipped := 0 diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 4afb102118..3a7fce3abb 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -103,6 +103,13 @@ func LefthookManaged(repoRoot string) bool { // EnsureLefthookIntegration registers Entire with Lefthook, returning the // number of artifacts written. // +// The hook-command prefix is read from this repository's settings rather than +// taken as an argument, for the reason ReinstallGitHooks exists: a caller that +// passes the wrong value does not fail, it silently drops +// absolute_git_hook_path — and here that means rewriting a working +// absolute-path install down to a bare `entire`, breaking delivery for the GUI +// git clients that setting exists to serve. `entire doctor` did exactly that. +// // There is deliberately no rollback. Every write is atomic and content- // idempotent, and the entry that activates the integration is written last, so // a failure part way through leaves inert files that the next install @@ -114,7 +121,7 @@ func LefthookManaged(repoRoot string) bool { // front, because it is a decision not to touch the repo rather than a failure // that will pass, and the exclude entries go in while the paths are still // empty, so a partial write is never left unignored. -func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, error) { +func EnsureLefthookIntegration(ctx context.Context) (int, error) { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil { return 0, fmt.Errorf("resolve worktree root: %w", err) @@ -138,7 +145,7 @@ func EnsureLefthookIntegration(ctx context.Context, absolutePath bool) (int, err // working as intended, and nothing here writes to it. return 0, fmt.Errorf("%w: %s", ErrLefthookLocalConfigTracked, name) } - cmdPrefix, err := hookCmdPrefix(absolutePath) + cmdPrefix, err := hookCmdPrefix(hookSettingsFromConfig(ctx)) if err != nil { return 0, err } @@ -325,7 +332,8 @@ func clearGitCommonDirCache() { // LefthookIntegrationCurrent reports whether Entire's artifacts are present // and current: its config, the extends entry pointing at it, and every script. -func LefthookIntegrationCurrent(ctx context.Context, absolutePath bool) (bool, error) { +// The prefix comes from settings; see EnsureLefthookIntegration. +func LefthookIntegrationCurrent(ctx context.Context) (bool, error) { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil { return false, fmt.Errorf("resolve worktree root: %w", err) @@ -345,7 +353,7 @@ func LefthookIntegrationCurrent(ctx context.Context, absolutePath bool) (bool, e if !extendsEntryPresent(root) { return false, nil } - cmdPrefix, err := hookCmdPrefix(absolutePath) + cmdPrefix, err := hookCmdPrefix(hookSettingsFromConfig(ctx)) if err != nil { return false, err } @@ -775,12 +783,12 @@ func uncoveredHookPaths(ctx context.Context) []string { // lefthookDeliversHooks reports whether Lefthook will run Entire from its own // configuration, which is what makes Entire's own hook files redundant. -func lefthookDeliversHooks(ctx context.Context, absolutePath bool) bool { +func lefthookDeliversHooks(ctx context.Context) bool { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil || !LefthookManaged(repoRoot) { return false } - current, err := LefthookIntegrationCurrent(ctx, absolutePath) + current, err := LefthookIntegrationCurrent(ctx) return err == nil && current } @@ -807,14 +815,14 @@ type HookDelivery struct { // A Lefthook repo is judged on the integration, not on .git/hooks/*: Lefthook // owns those files and rewrites them constantly, so their contents say nothing // about whether Entire will run. -func CheckHookDelivery(ctx context.Context, absolutePath bool) HookDelivery { +func CheckHookDelivery(ctx context.Context) HookDelivery { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil { return HookDelivery{Reason: "Could not resolve the repository root."} } declined := "" if LefthookManaged(repoRoot) { - current, checkErr := LefthookIntegrationCurrent(ctx, absolutePath) + current, checkErr := LefthookIntegrationCurrent(ctx) switch { case checkErr != nil: return HookDelivery{Manager: LefthookManagerName, diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 9b9dd1f5fa..132b9379b3 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -54,7 +54,7 @@ func TestEnsureLefthookIntegration(t *testing.T) { mainBefore, err := os.ReadFile(filepath.Join(dir, "lefthook.yml")) require.NoError(t, err) - written, err := EnsureLefthookIntegration(t.Context(), false) + written, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) require.Positive(t, written) @@ -82,12 +82,12 @@ func TestEnsureLefthookIntegration(t *testing.T) { require.NoError(t, err) require.Equal(t, mainBefore, mainAfter, "Lefthook's own config must not be touched") - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.True(t, current) // Repeat installs are no-ops. - written, err = EnsureLefthookIntegration(t.Context(), false) + written, err = EnsureLefthookIntegration(t.Context()) require.NoError(t, err) require.Zero(t, written) } @@ -98,9 +98,9 @@ func TestEnsureLefthookIntegration_MainConfigFormatIsIrrelevant(t *testing.T) { for _, name := range []string{"lefthook.toml", "lefthook.json", ".lefthook.yaml"} { t.Run(name, func(t *testing.T) { newLefthookRepo(t, name) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.True(t, current) }) @@ -120,7 +120,7 @@ func TestEnsureLefthookIntegration_PreservesLocalConfig(t *testing.T) { t.Run(tc.name, func(t *testing.T) { dir := newLefthookRepo(t, "") require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook-local.yml"), []byte(tc.local), 0o644)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) got, err := os.ReadFile(filepath.Join(dir, "lefthook-local.yml")) @@ -141,7 +141,7 @@ func TestEnsureLefthookIntegration_RefusesToShadowANonYAMLLocalConfig(t *testing original := []byte("[pre-commit.commands.mine]\nrun = \"true\"\n") require.NoError(t, os.WriteFile(tomlPath, original, 0o644)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.ErrorIs(t, err, ErrLefthookLocalConfigUnwritable) after, err := os.ReadFile(tomlPath) @@ -176,7 +176,7 @@ func TestEnsureLefthookIntegration_DisplacesUnownedFiles(t *testing.T) { theirConfig := []byte("# not Entire's\nkey: value\n") require.NoError(t, os.WriteFile(configPath, theirConfig, 0o644)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) backup, err := os.ReadFile(scriptPath + GitHookBackupSuffix) @@ -186,7 +186,7 @@ func TestEnsureLefthookIntegration_DisplacesUnownedFiles(t *testing.T) { require.NoError(t, err, "the displaced config must be kept") require.Equal(t, theirConfig, backup) - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.True(t, current, "and Entire's own artifacts installed over them") } @@ -196,7 +196,7 @@ func TestRemoveLefthookIntegration(t *testing.T) { dir := newLefthookRepo(t, "") localPath := filepath.Join(dir, "lefthook-local.yml") require.NoError(t, os.WriteFile(localPath, []byte("pre-commit:\n commands:\n mine:\n run: true\n"), 0o644)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) removed, err := RemoveLefthookIntegration(t.Context()) @@ -210,7 +210,7 @@ func TestRemoveLefthookIntegration(t *testing.T) { require.NotContains(t, string(local), entireLefthookConfig) require.Contains(t, string(local), "mine") - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.False(t, current) } @@ -252,7 +252,7 @@ func TestEnsureLefthookIntegration_ReconcilesHookFiles(t *testing.T) { write("post-commit.old", theirs) write("post-rewrite"+GitHookBackupSuffix, theirs) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) got, err := os.ReadFile(filepath.Join(hooksDir, "pre-push")) @@ -295,16 +295,16 @@ func TestEnsureLefthookIntegration_ReconcilesHookFiles(t *testing.T) { // owns can be present and correct while nothing dispatches Entire at all. func TestCheckHookDelivery_Lefthook(t *testing.T) { dir := newLefthookRepo(t, "") - got := CheckHookDelivery(t.Context(), false) + got := CheckHookDelivery(t.Context()) require.False(t, got.OK, "not registered yet") require.Equal(t, LefthookManagerName, got.Manager) require.NotEmpty(t, got.Reason) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) // Registered, but no hook file exists for any hook yet. - got = CheckHookDelivery(t.Context(), false) + got = CheckHookDelivery(t.Context()) require.False(t, got.OK, "a registration nothing triggers is not delivery") require.Equal(t, LefthookManagerName, got.Manager) for _, hook := range gitHookNames { @@ -321,7 +321,7 @@ func TestCheckHookDelivery_Lefthook(t *testing.T) { require.NoError(t, err) ClearHooksDirCache() - got = CheckHookDelivery(t.Context(), false) + got = CheckHookDelivery(t.Context()) require.True(t, got.OK, "reason: %s", got.Reason) require.Equal(t, LefthookManagerName, got.Manager) require.Empty(t, got.Reason) @@ -329,7 +329,7 @@ func TestCheckHookDelivery_Lefthook(t *testing.T) { // A hook file that belongs to neither is not delivery either. require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-push"), []byte("#!/bin/sh\necho someone else\n"), 0o755)) - got = CheckHookDelivery(t.Context(), false) + got = CheckHookDelivery(t.Context()) require.False(t, got.OK) require.Contains(t, got.Reason, "pre-push") } @@ -343,7 +343,7 @@ func TestCheckHookDelivery_Native(t *testing.T) { t.Cleanup(paths.ClearWorktreeRootCache) ClearHooksDirCache() - got := CheckHookDelivery(t.Context(), false) + got := CheckHookDelivery(t.Context()) require.False(t, got.OK) require.Empty(t, got.Manager) @@ -351,7 +351,7 @@ func TestCheckHookDelivery_Native(t *testing.T) { t.Fatalf("ReinstallGitHooks: %v", err) } ClearHooksDirCache() - got = CheckHookDelivery(t.Context(), false) + got = CheckHookDelivery(t.Context()) require.True(t, got.OK) require.Empty(t, got.Manager) } @@ -366,7 +366,7 @@ func TestEnsureLefthookIntegration_ExcludesArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(excludePath, []byte("# user's own\n/scratch\n"+excludeBlockBegin+"/stale-entry\n"+excludeBlockEnd), 0o644)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) got, err := os.ReadFile(excludePath) @@ -385,13 +385,13 @@ func TestEnsureLefthookIntegration_ExcludesArtifacts(t *testing.T) { // An install that predates an entry is not "current", so it repairs itself. require.NoError(t, os.WriteFile(excludePath, []byte(excludeBlockBegin+"/"+entireLefthookConfig+"\n"+excludeBlockEnd), 0o644)) - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.False(t, current, "a partial exclude block must trigger a repair") - _, err = EnsureLefthookIntegration(t.Context(), false) + _, err = EnsureLefthookIntegration(t.Context()) require.NoError(t, err) - current, err = LefthookIntegrationCurrent(t.Context(), false) + current, err = LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.True(t, current) @@ -444,7 +444,7 @@ func TestEnsureLefthookIntegration_RefusesToBuryABackup(t *testing.T) { older := []byte("#!/bin/sh\necho older\n") require.NoError(t, os.WriteFile(scriptPath+GitHookBackupSuffix, older, 0o755)) - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.ErrorIs(t, err, ErrLefthookArtifactBlocked) got, err := os.ReadFile(scriptPath + GitHookBackupSuffix) @@ -462,7 +462,7 @@ func TestEnsureLefthookIntegration_RefusesToBuryABackup(t *testing.T) { // A visible install that installed nothing must not claim otherwise. func TestInstallGitHook_ReportsLefthookDeliveryInsteadOfAFalseInstall(t *testing.T) { dir := newLefthookRepo(t, "") - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) hooksDir := filepath.Join(dir, ".git", "hooks") @@ -504,7 +504,7 @@ func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testi []byte("[pre-commit.commands.mine]\nrun = \"true\"\n"), 0o644)) ClearHooksDirCache() - got := CheckHookDelivery(t.Context(), false) + got := CheckHookDelivery(t.Context()) require.False(t, got.OK, "no hooks installed yet") require.Empty(t, got.Manager, "Lefthook is not the one delivering") require.Equal(t, "lefthook-local.toml is not YAML", got.Declined) @@ -514,7 +514,7 @@ func TestCheckHookDelivery_DeclinedLefthookConfigFallsBackToNativeHooks(t *testi require.NoError(t, err) ClearHooksDirCache() - got = CheckHookDelivery(t.Context(), false) + got = CheckHookDelivery(t.Context()) require.True(t, got.OK, "Entire's own hooks deliver here") require.Empty(t, got.Manager) require.Equal(t, "lefthook-local.toml is not YAML", got.Declined, "and the reason why is still reported") @@ -577,14 +577,14 @@ func TestEnsureLefthookIntegration_TrackedLocalConfig(t *testing.T) { testutil.GitAdd(t, dir, "lefthook-local.yml") testutil.GitCommit(t, dir, "team config") - _, err := EnsureLefthookIntegration(t.Context(), false) + _, err := EnsureLefthookIntegration(t.Context()) require.ErrorIs(t, err, ErrLefthookLocalConfigTracked) after, err := os.ReadFile(localPath) require.NoError(t, err) require.Equal(t, original, after, "a tracked config must not be modified") - got := CheckHookDelivery(t.Context(), false) + got := CheckHookDelivery(t.Context()) require.Empty(t, got.Manager, "Lefthook is not the one delivering") require.Equal(t, "lefthook-local.yml is tracked by git", got.Declined) @@ -592,9 +592,44 @@ func TestEnsureLefthookIntegration_TrackedLocalConfig(t *testing.T) { // Entire has nothing to write and the integration proceeds. require.NoError(t, os.WriteFile(localPath, append([]byte("extends:\n - "+entireLefthookConfig+"\n"), original...), 0o644)) - _, err = EnsureLefthookIntegration(t.Context(), false) + _, err = EnsureLefthookIntegration(t.Context()) require.NoError(t, err) - current, err := LefthookIntegrationCurrent(t.Context(), false) + current, err := LefthookIntegrationCurrent(t.Context()) require.NoError(t, err) require.True(t, current) } + +// A repo configured with absolute_git_hook_path must survive every entry +// point. doctor called these with a hardcoded false, so it reported a working +// install as NOT DELIVERING and then --force rewrote the scripts down to a +// bare `entire` — breaking delivery for the GUI git clients that setting +// exists to serve. The prefix now comes from settings, so no caller can +// choose it wrongly. +func TestLefthookIntegration_HonoursAbsoluteGitHookPath(t *testing.T) { + dir := newLefthookRepo(t, "") + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".entire"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, ".entire", "settings.json"), + []byte(`{"enabled": true, "absolute_git_hook_path": true}`), 0o644)) + + _, err := EnsureLefthookIntegration(t.Context()) + require.NoError(t, err) + + script, err := os.ReadFile(filepath.Join(dir, lefthookScriptPath("pre-push"))) + require.NoError(t, err) + require.NotContains(t, string(script), "if command -v entire >", + "an absolute-path repo must not get the bare command form: %s", script) + + current, err := LefthookIntegrationCurrent(t.Context()) + require.NoError(t, err) + require.True(t, current, "the install must read as current, not as needing repair") + + got := CheckHookDelivery(t.Context()) + require.Equal(t, LefthookManagerName, got.Manager) + + // The repair path must not rewrite it into the bare form either. + _, err = EnsureLefthookIntegration(t.Context()) + require.NoError(t, err) + after, err := os.ReadFile(filepath.Join(dir, lefthookScriptPath("pre-push"))) + require.NoError(t, err) + require.Equal(t, script, after, "a second install must not downgrade the command") +} diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go index f0e511d885..9ccd0980d1 100644 --- a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -115,7 +115,7 @@ func TestLefthookDeliversEntireOnWindows(t *testing.T) { run("git", "add", "a.txt") run("git", "commit", "-m", "init", "--no-verify") - _, err = EnsureLefthookIntegration(t.Context(), false) + _, err = EnsureLefthookIntegration(t.Context()) require.NoError(t, err) // Lefthook only generates a hook file per hook it knows about, and it // learns Entire's from the extends entry written above. From 7159e48be22ad207a76fd270c6c4de868d179b5a Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 11:50:44 -0700 Subject: [PATCH 08/18] docs: move the Lefthook key-file entry to checkpoint-implementation.md Main moved the strategy key-file list out of CLAUDE.md, so the rebase dropped this entry with the conflict. Same text, new home. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M37SP78XWXMRXA3E7BCBDSAS --- docs/development/checkpoint-implementation.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/development/checkpoint-implementation.md b/docs/development/checkpoint-implementation.md index f1e2353c35..48cb1a05c6 100644 --- a/docs/development/checkpoint-implementation.md +++ b/docs/development/checkpoint-implementation.md @@ -139,6 +139,35 @@ The manual-commit strategy (`manual_commit*.go`) does not modify the active bran - `cleanup.go` - Cleanup discovery/deletion for shadow branches, session states, and checkpoint metadata - `session_state.go` - Package-level session state functions - `hooks.go` - Git hook installation +- `lefthook.go` - Lefthook integration. Lefthook is the one hook manager that + does not just overwrite `.git/hooks/*` at install time but **reclaims every + hook file at the top of each run**, so Entire's turn-start self-heal always + loses the race: the file is taken back by the `pre-commit` of the very commit + being made. Owning the files is therefore not a fix, and Entire instead + registers with Lefthook itself — `entire-lefthook.yml` (Entire's own config, + declaring a script per hook), an `extends:` entry in `lefthook-local.yml` + pointing at it, and the scripts under `.lefthook-local//entire.sh`. + Lefthook resolves `extends` at **run time**, so this needs no `lefthook + install`. Load-bearing details: Lefthook *scripts* receive the hook's `$@` + (pre-push needs the remote and URL) while *commands* do not, so Entire uses + scripts; `lefthook-local.yml` shadows `lefthook-local.toml`, so Entire + refuses to create it beside a non-YAML local config + (`ErrLefthookLocalConfigUnwritable`) and keeps the native hooks instead; the + local config is edited as a `yaml.Node` so comments and key order survive; + artifacts are per-clone and generated, so they are ignored via + `.git/info/exclude` rather than the user's `.gitignore`; and every file + Entire did not write is backed up rather than replaced. `CheckHookDelivery` + is the single question `entire status` and `entire doctor` ask — in a + Lefthook repo it is answered by the registration, not by `.git/hooks/*`. + **Who owns the hook file** follows from that: once Lefthook dispatches + Entire, Entire owning the file too would run every hook twice, so + `installSkipsHook` leaves any hook carrying Lefthook's launcher alone and + `reconcileHookFiles` moves a launcher Entire had displaced back over its own + hook (and clears the `.old` backups that otherwise make every later + Lefthook sync fail, #1349). A hook Lefthook has *not* taken over is still + installed natively — in a repo where nobody ran `lefthook install`, Entire's + own hooks are the only delivery there is. This is also why `EnsureSetup` + runs the Lefthook step *before* the native install. Note: `checkpoint/configloader.go` overrides go-git's default config loader with a symlink-following `billy.Basic` (`osSymlinkFS`) — go-git's default reads config via `os.Root`, which rejects absolute symlinks in any path component (e.g. a `~/.config` managed by a dotfile tool), silently dropping global config so author identity fell back to "Unknown" and signing was skipped. From 4c8bb27b5a2b65d5a9b860d6f10bca8c414d6df5 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 11:52:11 -0700 Subject: [PATCH 09/18] fix(hooks): stop overriding the user's Lefthook script directory entire-lefthook.yml set source_dir_local so Lefthook would find Entire's scripts. A config reached through extends merges into the user's, so this overrode any source_dir_local the user had chosen: their own local scripts stopped resolving and Lefthook failed every commit with "script does not exist". Dropping the key alone would break Entire's scripts in those repos instead. Declare each hook as a command that runs Entire's script by path. {0} is the hook's arguments joined unescaped, which is exactly what Lefthook appends to a script's command line, so the scripts see the same "$@". Existing installs no longer match the rendered config and are rewritten on the next turn. The real-Lefthook test now keeps a user script in a custom directory and requires it to still run. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M37SRW0CWQ3KAFVFJYM33BBK --- cmd/entire/cli/strategy/lefthook.go | 33 +++++++++++++++---- cmd/entire/cli/strategy/lefthook_test.go | 4 ++- .../strategy/lefthook_windows_runner_test.go | 17 +++++++++- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 3a7fce3abb..09fb4a0897 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -16,9 +16,10 @@ package strategy // user action — and Lefthook's own config is never touched. // // Verified against lefthook 2.1.10: a config reached via `extends` can declare -// `scripts`, those scripts receive git's hook arguments (pre-push gets the -// remote and its URL), and the whole arrangement survives both -// `lefthook install -f` and a config edit. +// commands, a command receives git's hook arguments through `{0}` (pre-push +// gets the remote and its URL), and the whole arrangement survives both +// `lefthook install -f` and a config edit. See renderOwnedConfig for why these +// are commands and not `scripts:` entries. import ( "context" @@ -42,8 +43,9 @@ const ( // entireLefthookConfig is Entire's own Lefthook config. Entire owns this // file outright, which is what keeps it out of the user's. entireLefthookConfig = "entire-lefthook.yml" - // lefthookScriptDir is Lefthook's clone-local script directory, its - // default for source_dir_local. + // lefthookScriptDir holds Entire's hook scripts. It shares the name of + // Lefthook's default source_dir_local, but nothing depends on that: each + // command names its script by path. Kept for existing installs. lefthookScriptDir = ".lefthook-local" lefthookScript = "entire.sh" // lefthookOwnedMarker identifies a file as Entire's. Removal and @@ -439,14 +441,31 @@ func renderLefthookScript(spec hookSpec) string { "# "+entireHookMarker+"\n", "# "+lefthookOwnedMarker+"\n", 1) } +// renderOwnedConfig declares one Lefthook command per hook that runs Entire's +// script by its path. +// +// A command rather than a `scripts:` entry, because scripts are looked up under +// source_dir_local, and a config reached through `extends` merges into the +// user's: setting that key here overrode the user's own, so their local scripts +// stopped resolving and Lefthook failed every commit with "script does not +// exist". Leaving it unset breaks Entire's scripts instead whenever the user has +// chosen another directory. A command names its script outright and depends on +// neither. +// +// `{0}` is the hook's arguments joined by spaces, unescaped — exactly what +// Lefthook appends to a script's command line, so the script sees the same +// "$@" it did as a `scripts:` entry. Individual `{1}`/`{2}` placeholders are +// not used: an absent argument (prepare-commit-msg's source, for a plain +// commit) is left as literal text rather than dropped. func renderOwnedConfig() (string, error) { hooks := map[string]any{} for _, hook := range gitHookNames { hooks[hook] = map[string]any{ - "scripts": map[string]any{lefthookScript: map[string]any{"runner": "bash"}}, + "commands": map[string]any{"entire": map[string]any{ + "run": "bash " + lefthookScriptPath(hook) + " {0}", + }}, } } - hooks["source_dir_local"] = lefthookScriptDir out, err := yaml.Marshal(hooks) if err != nil { return "", fmt.Errorf("render %s: %w", entireLefthookConfig, err) diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 132b9379b3..1a4f86355c 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -61,7 +61,9 @@ func TestEnsureLefthookIntegration(t *testing.T) { owned, err := os.ReadFile(filepath.Join(dir, entireLefthookConfig)) require.NoError(t, err) require.Contains(t, string(owned), lefthookOwnedMarker) - require.Contains(t, string(owned), "source_dir_local: "+lefthookScriptDir) + require.NotContains(t, string(owned), "source_dir_local", + "an extended config's source_dir_local overrides the user's own") + require.Contains(t, string(owned), "bash "+lefthookScriptPath("pre-push")+" {0}") local, err := os.ReadFile(filepath.Join(dir, "lefthook-local.yml")) require.NoError(t, err) diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go index 9ccd0980d1..f986ec9d48 100644 --- a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -98,8 +98,20 @@ func TestLefthookDeliversEntireOnWindows(t *testing.T) { require.NoError(t, os.MkdirAll(binDir, 0o755)) record := filepath.Join(binDir, "args.txt") // hookCmdPrefix resolves to a bare "entire", so a stub on PATH stands in. + // post-rewrite also records its stdin: git passes the old/new pairs there, + // and Lefthook withholds stdin from a job unless it asks for it. require.NoError(t, os.WriteFile(filepath.Join(binDir, "entire"), - []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> "+shellQuote(filepath.ToSlash(record))+"\n"), 0o755)) + []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> "+shellQuote(filepath.ToSlash(record))+"\n"+ + "if [ \"$3\" = post-rewrite ]; then sed 's/^/stdin: /' >> "+shellQuote(filepath.ToSlash(record))+"; fi\n"), 0o755)) + + // The user's own Lefthook setup, with a script directory of their choosing. + // Entire's config merges into theirs, so it must not override it. + userRan := filepath.Join(binDir, "user-ran.txt") + require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook.yml"), + []byte("source_dir_local: .custom-local\npre-commit:\n scripts:\n user.sh:\n runner: bash\n"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".custom-local", "pre-commit"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, ".custom-local", "pre-commit", "user.sh"), + []byte("#!/bin/sh\necho ran > "+shellQuote(filepath.ToSlash(userRan))+"\n"), 0o755)) env := append(os.Environ(), "PATH="+binDir+string(os.PathListSeparator)+os.Getenv("PATH")) run := func(name string, args ...string) { @@ -131,4 +143,7 @@ func TestLefthookDeliversEntireOnWindows(t *testing.T) { for _, hook := range []string{"prepare-commit-msg", "commit-msg", "post-commit"} { require.Containsf(t, lines, hook, "Lefthook did not dispatch %s to Entire: %q", hook, got) } + _, err = os.Stat(userRan) + require.NoError(t, err, "the user's own Lefthook script must still run beside Entire's") + } From 78b35d091cfdb8e0873471786178bae40524b315 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 11:52:33 -0700 Subject: [PATCH 10/18] fix(hooks): give Entire's post-rewrite its stdin under Lefthook git passes post-rewrite's old/new commit pairs on stdin. Lefthook hands a job an empty stdin unless it sets use_stdin, so under Lefthook PostRewrite read no pairs and returned: every amend and rebase silently skipped the session remap. (It did not hang, as a review suggested; Lefthook's null reader returns EOF at once.) Set use_stdin on the post-rewrite command. The real-Lefthook test now amends and requires the pairs to reach Entire; it fails without this. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M37SSJC06Y3V7FEE647QHD6R --- cmd/entire/cli/strategy/lefthook.go | 12 ++++++++---- .../cli/strategy/lefthook_windows_runner_test.go | 8 ++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 09fb4a0897..f304dfd1a4 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -460,11 +460,15 @@ func renderLefthookScript(spec hookSpec) string { func renderOwnedConfig() (string, error) { hooks := map[string]any{} for _, hook := range gitHookNames { - hooks[hook] = map[string]any{ - "commands": map[string]any{"entire": map[string]any{ - "run": "bash " + lefthookScriptPath(hook) + " {0}", - }}, + command := map[string]any{"run": "bash " + lefthookScriptPath(hook) + " {0}"} + if hook == postRewriteHook { + // git passes post-rewrite's old/new pairs on stdin, and Lefthook + // gives a job an empty stdin unless it asks. Without this the hook + // runs, reads no pairs, and silently remaps nothing after every + // amend and rebase. + command["use_stdin"] = true } + hooks[hook] = map[string]any{"commands": map[string]any{"entire": command}} } out, err := yaml.Marshal(hooks) if err != nil { diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go index f986ec9d48..57a858f6f7 100644 --- a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -146,4 +146,12 @@ func TestLefthookDeliversEntireOnWindows(t *testing.T) { _, err = os.Stat(userRan) require.NoError(t, err, "the user's own Lefthook script must still run beside Entire's") + // Amend fires post-rewrite, whose old/new pairs arrive on stdin. Without + // them Entire has nothing to remap and silently does nothing. + run("git", "commit", "--amend", "--no-edit") + got, err = os.ReadFile(record) + require.NoError(t, err) + require.Contains(t, string(got), "hooks git post-rewrite amend", "post-rewrite was not dispatched") + require.Regexp(t, `(?m)^stdin: [0-9a-f]{40,64} [0-9a-f]{40,64}`, string(got), + "post-rewrite must receive git's rewrite pairs on stdin") } From 5820bf657cf1dc058dac83c2b5b09ed305e9ab4d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 12:48:42 -0700 Subject: [PATCH 11/18] fix(hooks): close Lefthook integration review gaps Entire-Checkpoint: 01M37X0CCQY73EJ6KKV5TB5EF0 --- cmd/entire/cli/strategy/common.go | 22 +++--- cmd/entire/cli/strategy/hooks.go | 20 +++++ cmd/entire/cli/strategy/lefthook.go | 39 +++++++--- cmd/entire/cli/strategy/lefthook_test.go | 75 ++++++++++++++++++- .../strategy/lefthook_windows_runner_test.go | 6 +- docs/development/checkpoint-implementation.md | 11 ++- 6 files changed, 144 insertions(+), 29 deletions(-) diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index 1dc16f689c..c433c56c77 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -116,12 +116,13 @@ func EnsureSetup(ctx context.Context) error { // .git/hooks/* is no fix in a Lefthook repo anyway: Lefthook takes those // files back at the top of its next run, which is typically the pre-commit // of the very commit being made. - if err := ensureLefthookIntegrationIfManaged(ctx); err != nil { + lefthookDelivers, err := ensureLefthookIntegrationIfManaged(ctx) + if err != nil { return err } // Install generic hooks (they delegate to strategy at runtime) - if !IsGitHookInstalled(ctx) { + if !nativeHooksCurrent(ctx, lefthookDelivers) { if _, err := ReinstallGitHooks(ctx); err != nil { return fmt.Errorf("failed to install git hooks: %w", err) } @@ -1819,32 +1820,33 @@ func prepareTranscriptIfNeeded(ctx context.Context, ag agent.Agent, transcriptPa } // ensureLefthookIntegrationIfManaged installs or repairs Entire's Lefthook -// registration when this repository is Lefthook-managed. A repo that declines +// registration when this repository is Lefthook-managed, and reports whether +// Lefthook now delivers Entire's hooks. A repo that declines // the integration (a non-YAML local config Entire will not shadow) keeps the // native hooks and is not an error. // // An already-current integration still reconciles the hook files, because // that is where a repo left mid-fight — Entire's hook chaining to Lefthook's // displaced launcher, running Entire twice — gets straightened out. -func ensureLefthookIntegrationIfManaged(ctx context.Context) error { +func ensureLefthookIntegrationIfManaged(ctx context.Context) (bool, error) { repoRoot, err := paths.WorktreeRoot(ctx) if err != nil || !LefthookManaged(repoRoot) { - return nil //nolint:nilerr // not a Lefthook repo: nothing to do + return false, nil //nolint:nilerr // not a Lefthook repo: nothing to do } current, err := LefthookIntegrationCurrent(ctx) if err != nil { - return nil //nolint:nilerr // inspection failure falls back to native hooks + return false, nil //nolint:nilerr // inspection failure falls back to native hooks } if current { - return reconcileHookFiles(ctx) + return true, reconcileHookFiles(ctx) } if _, err := EnsureLefthookIntegration(ctx); err != nil { // Both refusals are decisions about the user's repository, not // failures: the native hooks stay and status/doctor report why. if errors.Is(err, ErrLefthookLocalConfigUnwritable) || errors.Is(err, ErrLefthookLocalConfigTracked) { - return nil + return false, nil } - return fmt.Errorf("failed to register Entire with Lefthook: %w", err) + return false, fmt.Errorf("failed to register Entire with Lefthook: %w", err) } - return nil + return true, nil } diff --git a/cmd/entire/cli/strategy/hooks.go b/cmd/entire/cli/strategy/hooks.go index 854fa2f57e..63db93a302 100644 --- a/cmd/entire/cli/strategy/hooks.go +++ b/cmd/entire/cli/strategy/hooks.go @@ -493,6 +493,23 @@ const bareEntireHookCmd = "entire" // Current, which is what makes EnsureSetup reinstall it rather than leaving a // broken hook in place forever. func gitHookStateInHooksDir(hooksDir string) GitHookState { + return gitHookStateYielding(hooksDir, false) +} + +// nativeHooksCurrent reports whether the hook files Entire should own are +// current. When Lefthook delivers Entire, a hook carrying Lefthook's launcher +// is not Entire's to own (see installSkipsHook), so it is not counted as +// missing. Without that, a Lefthook repo read as "not installed" on every +// turn and EnsureSetup re-ran the whole native install to change nothing. +func nativeHooksCurrent(ctx context.Context, lefthookDelivers bool) bool { + hooksDir, err := GetHooksDir(ctx) + if err != nil { + return false + } + return gitHookStateYielding(hooksDir, lefthookDelivers) == GitHooksCurrent +} + +func gitHookStateYielding(hooksDir string, yieldToLefthook bool) GitHookState { // ForRemoval: this is a read, and reporting GitHooksAbsent for a symlinked // hooks directory is what sent EnsureSetup to InstallGitHook on every agent // turn, to fail on a refusal only `entire enable` should ever hit. Seeing @@ -503,6 +520,9 @@ func gitHookStateInHooksDir(hooksDir string) GitHookState { } outdated := false for _, hook := range gitHookNames { + if yieldToLefthook && isLefthookLauncher(root, hook, hook) { + continue + } // NoFollow: a hook that is a symlink is not one Entire wrote, whatever // is at the far end. Reporting Absent is what sends EnsureSetup to // InstallGitHook, which backs the link up and chains to it. diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index f304dfd1a4..68afa94729 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -399,19 +399,19 @@ func RemoveLefthookIntegration(ctx context.Context) (int, error) { } for _, hook := range gitHookNames { name := lefthookScriptPath(hook) - owned, err := fileIsOwned(root, name) - if err != nil || !owned { - continue // unreadable or not ours: leave it + wasRemoved, err := removeOwnedLefthookArtifact(root, name) + if err != nil { + return removed, err } - if err := osroot.RemoveNoSymlinks(root, name); err != nil && !os.IsNotExist(err) { - return removed, fmt.Errorf("remove %s: %w", name, err) + if wasRemoved { + removed++ } - removed++ } - if owned, ownErr := fileIsOwned(root, entireLefthookConfig); ownErr == nil && owned { - if err := osroot.RemoveNoSymlinks(root, entireLefthookConfig); err != nil && !os.IsNotExist(err) { - return removed, fmt.Errorf("remove %s: %w", entireLefthookConfig, err) - } + wasRemoved, err := removeOwnedLefthookArtifact(root, entireLefthookConfig) + if err != nil { + return removed, err + } + if wasRemoved { removed++ } if err := rewriteExcludeBlock(ctx, repoRoot, ""); err != nil { @@ -429,6 +429,25 @@ func RemoveLefthookIntegration(ctx context.Context) (int, error) { return removed, nil } +// removeOwnedLefthookArtifact removes a file Entire installed and restores the +// foreign file installation displaced to .pre-entire, when present. +// A file that no longer carries Entire's marker is left untouched together +// with its backup: uninstall must not replace content changed after install. +func removeOwnedLefthookArtifact(root *os.Root, name string) (bool, error) { + owned, err := fileIsOwned(root, name) + if err != nil || !owned { + return false, nil //nolint:nilerr // absent, unreadable, or foreign: leave it + } + if err := osroot.RemoveNoSymlinks(root, name); err != nil && !os.IsNotExist(err) { + return false, fmt.Errorf("remove %s: %w", name, err) + } + backup := name + GitHookBackupSuffix + if err := root.Rename(backup, name); err != nil && !os.IsNotExist(err) { + return true, fmt.Errorf("restore %s from %s: %w", name, backup, err) + } + return true, nil +} + func lefthookScriptPath(hook string) string { return filepath.ToSlash(filepath.Join(lefthookScriptDir, hook, lefthookScript)) } diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 1a4f86355c..ce76cbdc16 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -20,6 +20,7 @@ import ( func newLefthookRepo(t *testing.T, mainConfig string) string { t.Helper() dir := t.TempDir() + testutil.IsolateGitConfigEnv(t) testutil.InitRepo(t, dir) if mainConfig == "" { mainConfig = "lefthook.yml" @@ -47,6 +48,19 @@ func newLefthookRepo(t *testing.T, mainConfig string) string { return dir } +func TestNewLefthookRepo_IgnoresInheritedGitConfig(t *testing.T) { + outside := t.TempDir() + t.Setenv("GIT_CONFIG_COUNT", "1") + t.Setenv("GIT_CONFIG_KEY_0", "core.hooksPath") + t.Setenv("GIT_CONFIG_VALUE_0", outside) + + newLefthookRepo(t, "") + hooksDir, err := GetHooksDir(t.Context()) + require.NoError(t, err) + require.Equal(t, filepath.Join(".git", "hooks"), hooksDir, + "test repositories must not inherit the developer's core.hooksPath") +} + // Install writes Entire's own config, one extends entry, and a script per // hook — and nothing into Lefthook's own config. func TestEnsureLefthookIntegration(t *testing.T) { @@ -198,6 +212,13 @@ func TestRemoveLefthookIntegration(t *testing.T) { dir := newLefthookRepo(t, "") localPath := filepath.Join(dir, "lefthook-local.yml") require.NoError(t, os.WriteFile(localPath, []byte("pre-commit:\n commands:\n mine:\n run: true\n"), 0o644)) + configPath := filepath.Join(dir, entireLefthookConfig) + userConfig := []byte("# user's config\npre-commit: {}\n") + require.NoError(t, os.WriteFile(configPath, userConfig, 0o644)) + scriptPath := filepath.Join(dir, lefthookScriptPath("pre-push")) + require.NoError(t, os.MkdirAll(filepath.Dir(scriptPath), 0o755)) + userScript := []byte("#!/bin/sh\necho user hook\n") + require.NoError(t, os.WriteFile(scriptPath, userScript, 0o755)) _, err := EnsureLefthookIntegration(t.Context()) require.NoError(t, err) @@ -205,8 +226,16 @@ func TestRemoveLefthookIntegration(t *testing.T) { require.NoError(t, err) require.Positive(t, removed) - _, statErr := os.Stat(filepath.Join(dir, entireLefthookConfig)) - require.True(t, os.IsNotExist(statErr)) + restoredConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + require.Equal(t, userConfig, restoredConfig, "the displaced config must be restored") + _, statErr := os.Stat(configPath + GitHookBackupSuffix) + require.True(t, os.IsNotExist(statErr), "the restored config backup must be consumed") + restoredScript, err := os.ReadFile(scriptPath) + require.NoError(t, err) + require.Equal(t, userScript, restoredScript, "the displaced script must be restored") + _, statErr = os.Stat(scriptPath + GitHookBackupSuffix) + require.True(t, os.IsNotExist(statErr), "the restored script backup must be consumed") local, err := os.ReadFile(localPath) require.NoError(t, err, "the user's local config must survive") require.NotContains(t, string(local), entireLefthookConfig) @@ -223,12 +252,18 @@ func TestRemoveLefthookIntegration_LeavesForeignFiles(t *testing.T) { configPath := filepath.Join(dir, entireLefthookConfig) theirs := []byte("# not Entire's\nkey: value\n") require.NoError(t, os.WriteFile(configPath, theirs, 0o644)) + backup := []byte("# displaced before a later user edit\n") + require.NoError(t, os.WriteFile(configPath+GitHookBackupSuffix, backup, 0o644)) _, err := RemoveLefthookIntegration(t.Context()) require.NoError(t, err) after, err := os.ReadFile(configPath) require.NoError(t, err) require.Equal(t, theirs, after) + afterBackup, err := os.ReadFile(configPath + GitHookBackupSuffix) + require.NoError(t, err) + require.Equal(t, backup, afterBackup, + "a later user edit must not be replaced by an older backup") } // Two kinds of wreckage from the era when Entire and Lefthook fought over @@ -635,3 +670,39 @@ func TestLefthookIntegration_HonoursAbsoluteGitHookPath(t *testing.T) { require.NoError(t, err) require.Equal(t, script, after, "a second install must not downgrade the command") } + +// In a repo where Lefthook delivers Entire, a hook holding Lefthook's +// launcher is Lefthook's, so it must not read as a missing Entire hook — +// otherwise EnsureSetup re-ran the full native install on every turn to +// change nothing. A hook with no file at all still needs Entire's. +func TestNativeHooksCurrent_YieldsToLefthookLaunchers(t *testing.T) { + dir := newLefthookRepo(t, "") + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + for _, h := range gitHookNames { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, h), lefthookLauncher(h), 0o755)) + } + ctx := t.Context() + + delivers, err := ensureLefthookIntegrationIfManaged(ctx) + require.NoError(t, err) + require.True(t, delivers) + require.False(t, IsGitHookInstalled(ctx), "no hook carries Entire's own marker") + require.True(t, nativeHooksCurrent(ctx, true), "every hook is Lefthook's to own") + require.False(t, nativeHooksCurrent(ctx, false), "without Lefthook delivering, launchers are not Entire's hooks") + + var nativePostCommit []byte + for _, spec := range buildHookSpecs(bareEntireHookCmd) { + if spec.name == "post-commit" { + nativePostCommit = []byte(spec.content) + break + } + } + require.NotEmpty(t, nativePostCommit) + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "post-commit"), nativePostCommit, 0o755)) + require.True(t, nativeHooksCurrent(ctx, true), + "a mixed set of Lefthook launchers and current native hooks is current") + + require.NoError(t, os.Remove(filepath.Join(hooksDir, "pre-push"))) + require.False(t, nativeHooksCurrent(ctx, true), "a hook with no file must still be installed") +} diff --git a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go index 57a858f6f7..08c5d6215f 100644 --- a/cmd/entire/cli/strategy/lefthook_windows_runner_test.go +++ b/cmd/entire/cli/strategy/lefthook_windows_runner_test.go @@ -14,9 +14,9 @@ import ( // TestLefthookScriptRunsUnderItsRunnerOnWindows exercises the one assumption // in the Lefthook integration that does not hold by construction. // -// Entire's hooks reach Lefthook as `scripts` declared with `runner: bash`, so -// Lefthook resolves bash through PATH and spawns it with the script. Every -// other artifact is a file Entire writes and reads itself, but this step is +// Entire's hooks reach Lefthook as commands that invoke their scripts through +// bash, so Lefthook resolves bash through PATH and spawns it with the script. +// Every other artifact is a file Entire writes and reads itself, but this step is // Lefthook executing a POSIX script on a platform whose shell is not POSIX — // and if bash does not resolve, or the script does not run under it, Entire // silently stops capturing in every Lefthook repo on Windows. diff --git a/docs/development/checkpoint-implementation.md b/docs/development/checkpoint-implementation.md index 48cb1a05c6..103b38d9a3 100644 --- a/docs/development/checkpoint-implementation.md +++ b/docs/development/checkpoint-implementation.md @@ -145,12 +145,15 @@ The manual-commit strategy (`manual_commit*.go`) does not modify the active bran loses the race: the file is taken back by the `pre-commit` of the very commit being made. Owning the files is therefore not a fix, and Entire instead registers with Lefthook itself — `entire-lefthook.yml` (Entire's own config, - declaring a script per hook), an `extends:` entry in `lefthook-local.yml` + declaring a command per hook), an `extends:` entry in `lefthook-local.yml` pointing at it, and the scripts under `.lefthook-local//entire.sh`. Lefthook resolves `extends` at **run time**, so this needs no `lefthook - install`. Load-bearing details: Lefthook *scripts* receive the hook's `$@` - (pre-push needs the remote and URL) while *commands* do not, so Entire uses - scripts; `lefthook-local.yml` shadows `lefthook-local.toml`, so Entire + install`. Load-bearing details: each command names Entire's script directly, + avoiding the merged `source_dir_local` setting that would otherwise override + the user's script directory, and `{0}` forwards the hook's arguments + (pre-push needs the remote and URL); `post-rewrite` also sets `use_stdin` so + Git's rewrite pairs reach Entire; `lefthook-local.yml` shadows + `lefthook-local.toml`, so Entire refuses to create it beside a non-YAML local config (`ErrLefthookLocalConfigUnwritable`) and keeps the native hooks instead; the local config is edited as a `yaml.Node` so comments and key order survive; From 7c5edad403bd3ed6044c7ab3eb9ff27e00caf480 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 14:26:54 -0700 Subject: [PATCH 12/18] fix(hooks): coalesce common-dir cache misses Entire-Checkpoint: 01M382M5H75WAH035FJBHZ7DWQ --- cmd/entire/cli/strategy/lefthook.go | 34 +++++++++++++--- cmd/entire/cli/strategy/lefthook_test.go | 49 ++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 68afa94729..7d7bf680ae 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -36,6 +36,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/worktreedir" + "golang.org/x/sync/singleflight" "gopkg.in/yaml.v3" ) @@ -306,6 +307,10 @@ func openGitCommonDir(ctx context.Context, repoRoot string) (*os.Root, error) { var ( gitCommonDirMu sync.RWMutex gitCommonDirCache = map[string]string{} + gitCommonDirCalls singleflight.Group + + // Test seam for proving that concurrent misses share one subprocess. + gitCommonDirResolve = gitdir.CommonDirForWorktree ) func gitCommonDirFor(ctx context.Context, repoRoot string) (string, error) { @@ -315,13 +320,32 @@ func gitCommonDirFor(ctx context.Context, repoRoot string) (string, error) { if ok { return cached, nil } - commonDir, err := gitdir.CommonDirForWorktree(ctx, repoRoot) + value, err, _ := gitCommonDirCalls.Do(repoRoot, func() (any, error) { + // A preceding call may have populated the cache before this keyed call + // acquired single-flight ownership. + gitCommonDirMu.RLock() + cached, ok := gitCommonDirCache[repoRoot] + gitCommonDirMu.RUnlock() + if ok { + return cached, nil + } + + commonDir, err := gitCommonDirResolve(ctx, repoRoot) + if err != nil { + return "", fmt.Errorf("resolve git common dir: %w", err) + } + gitCommonDirMu.Lock() + gitCommonDirCache[repoRoot] = commonDir + gitCommonDirMu.Unlock() + return commonDir, nil + }) if err != nil { - return "", fmt.Errorf("resolve git common dir: %w", err) + return "", fmt.Errorf("coalesce git common dir lookup: %w", err) + } + commonDir, ok := value.(string) + if !ok { + return "", errors.New("coalesced git common dir lookup returned a non-string value") } - gitCommonDirMu.Lock() - gitCommonDirCache[repoRoot] = commonDir - gitCommonDirMu.Unlock() return commonDir, nil } diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index ce76cbdc16..31f6105886 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -1,11 +1,13 @@ package strategy import ( + "context" "io" "os" "path/filepath" "strings" "testing" + "time" "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -61,6 +63,53 @@ func TestNewLefthookRepo_IgnoresInheritedGitConfig(t *testing.T) { "test repositories must not inherit the developer's core.hooksPath") } +func TestGitCommonDirFor_CoalescesConcurrentMisses(t *testing.T) { + clearGitCommonDirCache() + originalResolve := gitCommonDirResolve + t.Cleanup(func() { + gitCommonDirResolve = originalResolve + clearGitCommonDirCache() + }) + + const callers = 16 + entered := make(chan struct{}, callers) + release := make(chan struct{}) + gitCommonDirResolve = func(_ context.Context, repoRoot string) (string, error) { + entered <- struct{}{} + <-release + return filepath.Join(repoRoot, ".git"), nil + } + + start := make(chan struct{}) + ready := make(chan struct{}, callers) + results := make(chan error, callers) + for range callers { + go func() { + ready <- struct{}{} + <-start + _, err := gitCommonDirFor(t.Context(), "/repo") + results <- err + }() + } + for range callers { + <-ready + } + close(start) + <-entered + + duplicate := false + select { + case <-entered: + duplicate = true + case <-time.After(200 * time.Millisecond): + } + close(release) + for range callers { + require.NoError(t, <-results) + } + require.False(t, duplicate, "concurrent misses must share one git common-dir lookup") +} + // Install writes Entire's own config, one extends entry, and a script per // hook — and nothing into Lefthook's own config. func TestEnsureLefthookIntegration(t *testing.T) { From f69d44486336d2e9a80361046a9c71e554c8584f Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 16:09:10 -0700 Subject: [PATCH 13/18] fix(hooks): honor Lefthook local config forms Entire-Checkpoint: 01M388FDS46YXDYX0F9D6RQ5SW --- cmd/entire/cli/strategy/lefthook.go | 52 ++++++++++++++++-------- cmd/entire/cli/strategy/lefthook_test.go | 20 ++++++++- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 7d7bf680ae..81485af172 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -140,13 +140,15 @@ func EnsureLefthookIntegration(ctx context.Context) (int, error) { // the point of excluding — and because ensureLefthookIntegrationIfManaged // treats this refusal as a decision rather than a failure, that recurred // on every turn instead of resolving itself. - if name, existing, err := findLocalConfig(root); err != nil { + localConfigName, existing, err := findLocalConfig(root) + if err != nil { return 0, err - } else if existing != nil && !extendsEntryPresent(root) && lefthookLocalConfigTracked(ctx, repoRoot, name) { + } + if existing != nil && !extendsEntryPresent(root) && lefthookLocalConfigTracked(ctx, repoRoot, localConfigName) { // Only when the entry would actually be ADDED: a tracked config that // already carries it (committed by the team, or added by hand) is // working as intended, and nothing here writes to it. - return 0, fmt.Errorf("%w: %s", ErrLefthookLocalConfigTracked, name) + return 0, fmt.Errorf("%w: %s", ErrLefthookLocalConfigTracked, localConfigName) } cmdPrefix, err := hookCmdPrefix(hookSettingsFromConfig(ctx)) if err != nil { @@ -157,7 +159,7 @@ func EnsureLefthookIntegration(ctx context.Context) (int, error) { // blocked artifact path, a full disk) otherwise leaves files in the // worktree that nothing has excluded yet, and the entries name paths // rather than existing files, so writing them early costs nothing. - if err := excludeArtifacts(ctx, repoRoot); err != nil { + if err := excludeArtifacts(ctx, repoRoot, localConfigName); err != nil { return 0, err } @@ -202,27 +204,28 @@ func EnsureLefthookIntegration(ctx context.Context) (int, error) { // artifactsExcluded reports whether the exclude block is in place, which the // install treats as part of being current. -func artifactsExcluded(ctx context.Context, repoRoot string) bool { +func artifactsExcluded(ctx context.Context, repoRoot, localConfigName string) bool { root, err := openGitCommonDir(ctx, repoRoot) if err != nil { return true // cannot tell; do not churn the install on it } data, err := osroot.ReadFileNoFollow(root, gitExcludePath) - return err == nil && strings.Contains(string(data), lefthookExcludeBlock()) + return err == nil && strings.Contains(string(data), lefthookExcludeBlock(localConfigName)) } // lefthookExcludeBlock is the exact text excludeArtifacts writes, so // artifactsExcluded tests for what an install would produce and any future // change to the entries repairs itself on the next turn. -func lefthookExcludeBlock() string { - // lefthook-local.yml is included because Entire creates it when absent and - // Lefthook documents it as a personal, uncommitted file. An exclude entry - // only affects untracked paths, so a repository that does commit it is - // unaffected by this. +func lefthookExcludeBlock(localConfigName string) string { + // The selected local config is included because Entire creates it when + // absent or modifies the first existing name in Lefthook's precedence + // order. Lefthook documents these as personal, uncommitted files. An + // exclude entry only affects untracked paths, so a repository that does + // commit one is unaffected by this. return excludeBlockBegin + "/" + entireLefthookConfig + "\n" + "/" + lefthookScriptDir + "/\n" + - "/" + lefthookLocalConfigNames[0] + "\n" + + "/" + localConfigName + "\n" + excludeBlockEnd } @@ -232,6 +235,7 @@ const ( // for pulling in another config file at run time. LefthookManagerName = "Lefthook" lefthookExtendsKey = "extends" + yamlStringTag = "!!str" gitExcludePath = "info/exclude" excludeBlockBegin = "# " + lefthookOwnedMarker + " begin\n" @@ -243,8 +247,8 @@ const ( // in the user's .gitignore — and left unignored they make a dirty worktree the // normal state in every Lefthook repo, which eventually gets Entire's // integration files committed by an agent that sees them in git status. -func excludeArtifacts(ctx context.Context, repoRoot string) error { - return rewriteExcludeBlock(ctx, repoRoot, lefthookExcludeBlock()) +func excludeArtifacts(ctx context.Context, repoRoot, localConfigName string) error { + return rewriteExcludeBlock(ctx, repoRoot, lefthookExcludeBlock(localConfigName)) } // rewriteExcludeBlock replaces Entire's marked block in .git/info/exclude with @@ -376,6 +380,10 @@ func LefthookIntegrationCurrent(ctx context.Context) (bool, error) { if err != nil || string(data) != want { return false, nil //nolint:nilerr // a missing or stale config is "not current", not a failure } + localConfigName, _, err := findLocalConfig(root) + if err != nil { + return false, nil //nolint:nilerr // an unsupported local config cannot be current + } if !extendsEntryPresent(root) { return false, nil } @@ -392,7 +400,7 @@ func LefthookIntegrationCurrent(ctx context.Context) (bool, error) { // The exclude entry counts as part of the install. Without it every // Lefthook worktree reads as dirty, and a repo that predates this check // needs a repair pass to pick it up. - return artifactsExcluded(ctx, repoRoot), nil + return artifactsExcluded(ctx, repoRoot, localConfigName), nil } // RemoveLefthookIntegration removes Entire's artifacts, returning the number @@ -598,7 +606,7 @@ func ensureExtendsEntry(root *os.Root) (string, error) { } } seq.Content = append(seq.Content, &yaml.Node{ - Kind: yaml.ScalarNode, Tag: "!!str", Value: entireLefthookConfig, + Kind: yaml.ScalarNode, Tag: yamlStringTag, Value: entireLefthookConfig, LineComment: lefthookOwnedMarker, }) out, err := encodeYAML(doc) @@ -705,6 +713,16 @@ func extendsSequence(existing []byte) (*yaml.Node, *yaml.Node, error) { } if found >= 0 { seq := mapping.Content[found+1] + if seq.Kind == yaml.ScalarNode && seq.Tag == yamlStringTag { + seq = &yaml.Node{ + Kind: yaml.SequenceNode, + Tag: "!!seq", + Content: []*yaml.Node{ + mapping.Content[found+1], + }, + } + mapping.Content[found+1] = seq + } if seq.Kind != yaml.SequenceNode { return nil, nil, errors.New("extends must be a sequence") } @@ -712,7 +730,7 @@ func extendsSequence(existing []byte) (*yaml.Node, *yaml.Node, error) { } seq := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"} mapping.Content = append(mapping.Content, - &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: lefthookExtendsKey}, seq) + &yaml.Node{Kind: yaml.ScalarNode, Tag: yamlStringTag, Value: lefthookExtendsKey}, seq) return doc, seq, nil } diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 31f6105886..8113ec93a0 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -181,6 +181,7 @@ func TestEnsureLefthookIntegration_PreservesLocalConfig(t *testing.T) { {"existing content", "# keep me\npre-commit:\n commands:\n mine:\n run: true\n", []string{"# keep me", "mine"}}, {"existing extends", "extends:\n - user.yml\n", []string{"user.yml"}}, + {"scalar extends", "extends: user.yml\n", []string{"user.yml"}}, } { t.Run(tc.name, func(t *testing.T) { dir := newLefthookRepo(t, "") @@ -494,6 +495,23 @@ func TestEnsureLefthookIntegration_ExcludesArtifacts(t *testing.T) { require.True(t, os.IsNotExist(statErr), "the empty script dir must be pruned") } +func TestEnsureLefthookIntegration_ExcludesSelectedLocalConfig(t *testing.T) { + dir := newLefthookRepo(t, "") + localConfig := lefthookLocalConfigNames[1] + require.NoError(t, os.WriteFile(filepath.Join(dir, localConfig), []byte("pre-commit: {}\n"), 0o644)) + + _, err := EnsureLefthookIntegration(t.Context()) + require.NoError(t, err) + + exclude, err := os.ReadFile(filepath.Join(dir, ".git", "info", "exclude")) + require.NoError(t, err) + require.Contains(t, string(exclude), "/"+localConfig+"\n") + + current, err := LefthookIntegrationCurrent(t.Context()) + require.NoError(t, err) + require.True(t, current) +} + // Entire creates lefthook-local.yml itself, so it must never be the thing that // makes a repo look Lefthook-managed — otherwise removing Lefthook leaves // status claiming delivery "via Lefthook" with nothing running Entire. @@ -541,7 +559,7 @@ func TestEnsureLefthookIntegration_RefusesToBuryABackup(t *testing.T) { // manage to write must already be excluded. exclude, err := os.ReadFile(filepath.Join(dir, ".git", "info", "exclude")) require.NoError(t, err) - require.Contains(t, string(exclude), lefthookExcludeBlock(), + require.Contains(t, string(exclude), lefthookExcludeBlock(lefthookLocalConfigNames[0]), "a partial install must not leave unignored artifacts") } From 69318001035b4f3f8d2cdf9cb949cb25eb7ab8c8 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 17:12:51 -0700 Subject: [PATCH 14/18] test(status): cover missing native hook paths Entire-Checkpoint: 01M38C4143ERYFH2TNVNGRZA88 --- cmd/entire/cli/status_test.go | 71 +++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 2d44639b02..2c93836da9 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -3345,3 +3345,74 @@ func TestRunStatus_HookDeliveryLine(t *testing.T) { t.Errorf("hooks fields = %+v, want delivering via Lefthook: %s", got, jsonOut.String()) } } + +// #2264 is not Lefthook-specific: whatever makes the native hooks unreachable, +// status must qualify its checkpoint destination with a capture warning. These +// cases pin the distinct repo states from the issue's acceptance matrix. +func TestRunStatus_NativeHookDeliveryFailures(t *testing.T) { + for _, tc := range []struct { + name string + arrangeFailure func(t *testing.T, dir string) + }{ + { + name: "fresh enabled clone", + arrangeFailure: func(*testing.T, string) { + // Enabled settings arrived with the clone, but its .git directory + // is local and has never had Entire's hooks installed. + }, + }, + { + name: "native hook removed", + arrangeFailure: func(t *testing.T, dir string) { + if _, err := strategy.ReinstallGitHooks(t.Context()); err != nil { + t.Fatalf("ReinstallGitHooks: %v", err) + } + if err := os.Remove(filepath.Join(dir, ".git", "hooks", "pre-push")); err != nil { + t.Fatalf("remove pre-push: %v", err) + } + }, + }, + { + name: "core hooks path redirected", + arrangeFailure: func(t *testing.T, dir string) { + if _, err := strategy.ReinstallGitHooks(t.Context()); err != nil { + t.Fatalf("ReinstallGitHooks: %v", err) + } + testutil.RunGit(t, dir, "config", "core.hooksPath", filepath.Join(dir, "other-hooks")) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + dir := setupTestRepo(t) + writeSettings(t, testSettingsEnabled) + testutil.RunGit(t, dir, "remote", "add", "origin", "https://example.invalid/repo.git") + tc.arrangeFailure(t, dir) + strategy.ClearHooksDirCache() + + var stdout bytes.Buffer + if err := runStatus(t.Context(), &stdout, false, false); err != nil { + t.Fatalf("runStatus: %v", err) + } + out := stdout.String() + if !strings.Contains(out, "Checkpoints are NOT being captured") { + t.Errorf("status must warn when hooks do not deliver, got:\n%s", out) + } + if !strings.Contains(out, "Checkpoints sync to: origin") { + t.Errorf("status must retain the diagnostic destination, got:\n%s", out) + } + + var jsonOut bytes.Buffer + if err := runStatusJSON(t.Context(), &jsonOut); err != nil { + t.Fatalf("runStatusJSON: %v", err) + } + var got statusJSON + if err := json.Unmarshal(jsonOut.Bytes(), &got); err != nil { + t.Fatalf("unmarshal status JSON: %v", err) + } + if got.HooksDeliver || got.HooksManager != "" || got.HooksDeliverReason == "" { + t.Errorf("hooks fields = %+v, want failed native delivery: %s", got, jsonOut.String()) + } + }) + } +} From 336b4648206070c1663dddabd2d6bda79ee6e9c6 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 20:07:56 -0700 Subject: [PATCH 15/18] fix(status): stop promising a push will sync when no hook runs it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With Entire's Git hooks not delivering, status warned that checkpoints are not being captured but still told the user their pending checkpoints "sync with your next 'git push origin'" — which #2264 calls out as a promise that cannot happen, since no pre-push hook will run. The unpushed counter now keeps the count and replaces the promise with "won't sync until Entire's Git hooks run again" whenever hook delivery fails. The pushing-disabled phrasing already promised nothing and is unchanged. Fixtures that assert the promise now install the hooks first. Co-Authored-By: Claude Opus 5.5 (1M context) Entire-Checkpoint: 01M38P4MC6TJMWSECRGGDDRA90 --- cmd/entire/cli/status.go | 32 +++++++++++++++++++++++++------- cmd/entire/cli/status_test.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index d1f6f4b81f..6627fcdf15 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -247,8 +247,8 @@ func formatSettingsStatusShort(ctx context.Context, s *EntireSettings, sty statu // Where checkpoint data syncs (the single elected remote), and how many // checkpoints have not reached it yet. Local-only computation. if s.Enabled { - writeHookDeliveryLine(ctx, &b, sty) - writeCheckpointSyncLines(ctx, &b, s, sty) + hooksDeliver := writeHookDeliveryLine(ctx, &b, sty) + writeCheckpointSyncLines(ctx, &b, s, sty, hooksDeliver) } if s.Enabled { @@ -393,6 +393,10 @@ type checkpointSyncInfo struct { // where a user finds out why — the hooks only log the rejection. IgnoredRemote string IgnoredReason string + // HooksNotDelivering reports that no Git hook will run Entire, so a push + // syncs nothing and the unpushed counter must not say it will (#2264). + // Text path only: JSON reports delivery through its own hooks_* fields. + HooksNotDelivering bool } // resolveDedicatedReadSource records where checkpoint READS land when the @@ -568,8 +572,9 @@ func countUnpushedCheckpointsForStatus(ctx context.Context, remoteName string) i // Without it status named a checkpoint destination while no hook existed to // reach it, which is issue #2264. The destination line below is still shown — // it is true, and status is the only place it appears — but this line above it -// removes the claim that delivery is working. -func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusStyles) { +// removes the claim that delivery is working. It returns whether the hooks +// deliver, so the counter below can drop its promise about the next push. +func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusStyles) bool { delivery := strategy.CheckHookDelivery(ctx) b.WriteString("\n") switch { @@ -581,6 +586,7 @@ func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusSt b.WriteString(sty.render(sty.yellow, " ! Checkpoints are NOT being captured: "+delivery.Reason)) b.WriteString(sty.render(sty.dim, " · run 'entire doctor'")) } + return delivery.OK } // writeCheckpointSyncLines reports the checkpoint sync destination (and the @@ -592,8 +598,9 @@ func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusSt // pushing off the elected remote is still the read source and the counter still // reports local-only data, so the lines are reworded rather than dropped — // status is the only surface that names either. -func writeCheckpointSyncLines(ctx context.Context, b *strings.Builder, s *EntireSettings, sty statusStyles) { +func writeCheckpointSyncLines(ctx context.Context, b *strings.Builder, s *EntireSettings, sty statusStyles, hooksDeliver bool) { info := computeCheckpointSyncInfo(ctx, s) + info.HooksNotDelivering = !hooksDeliver destination := "\n Checkpoints sync to: " if info.PushDisabled { b.WriteString("\n Automatic checkpoint pushing: disabled") @@ -672,12 +679,15 @@ func writeCheckpointSyncLines(ctx context.Context, b *strings.Builder, s *Entire // destination — and never that the data exists nowhere else. Getting this // backwards would falsely reassure someone asking whether checkpoint data has // left the machine. +// +// With pushing enabled but no hook delivering, the future tense is false for a +// different reason: no pre-push hook will run, so the next push syncs nothing. func formatUnpushedCheckpointsLine(info checkpointSyncInfo) string { noun := nounCheckpoints - pronoun := "they sync" + pronoun, notPronoun := "they sync", "they won't sync" if info.Unpushed == 1 { noun = nounCheckpoint - pronoun = "it syncs" + pronoun, notPronoun = "it syncs", "it won't sync" } if info.PushDisabled { if info.Source == checkpointSyncSourceDedicated { @@ -685,6 +695,14 @@ func formatUnpushedCheckpointsLine(info checkpointSyncInfo) string { } return fmt.Sprintf("%d %s not on %s", info.Unpushed, noun, info.Remote) } + if info.HooksNotDelivering { + // The warning above names the cause; here only the promise goes. + pending := fmt.Sprintf("%d %s not yet on %s", info.Unpushed, noun, info.Remote) + if info.Source == checkpointSyncSourceDedicated { + pending = fmt.Sprintf("%d %s not yet pushed", info.Unpushed, noun) + } + return fmt.Sprintf("%s — %s until Entire's Git hooks run again", pending, notPronoun) + } if info.Source == checkpointSyncSourceDedicated { return fmt.Sprintf("%d %s not yet pushed", info.Unpushed, noun) } diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 2c93836da9..1d943421a7 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -2470,7 +2470,7 @@ func assertCheckpointPushDisabledStatus(t *testing.T, jsonOutput, detailed bool, } } -// formatUnpushedCheckpointsLine is pure, so its four branches are pinned here +// formatUnpushedCheckpointsLine is pure, so its branches are pinned here // rather than through a repo fixture. What the counter must never say with // pushing disabled is that the data is local-only: Unpushed compares against // the elected destination alone, so it cannot establish that checkpoints exist @@ -2491,6 +2491,16 @@ func TestFormatUnpushedCheckpointsLine(t *testing.T) { "1 checkpoint not on origin"}, {"pushing_disabled_dedicated", checkpointSyncInfo{PushDisabled: true, Remote: "org/cp", Source: checkpointSyncSourceDedicated, Unpushed: 2}, "2 checkpoints not pushed to the checkpoint remote"}, + // #2264: with no hook to run pre-push, "your next push" syncs nothing. + {"hooks_not_delivering", checkpointSyncInfo{Remote: "origin", Source: "default", Unpushed: 2, HooksNotDelivering: true}, + "2 checkpoints not yet on origin — they won't sync until Entire's Git hooks run again"}, + {"hooks_not_delivering_singular", checkpointSyncInfo{Remote: "origin", Source: "default", Unpushed: 1, HooksNotDelivering: true}, + "1 checkpoint not yet on origin — it won't sync until Entire's Git hooks run again"}, + {"hooks_not_delivering_dedicated", checkpointSyncInfo{Remote: "org/cp", Source: checkpointSyncSourceDedicated, Unpushed: 2, HooksNotDelivering: true}, + "2 checkpoints not yet pushed — they won't sync until Entire's Git hooks run again"}, + // Pushing disabled already promises nothing, so the hooks change nothing. + {"hooks_not_delivering_pushing_disabled", checkpointSyncInfo{PushDisabled: true, Remote: "origin", Source: "default", Unpushed: 1, HooksNotDelivering: true}, + "1 checkpoint not on origin"}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -2786,6 +2796,7 @@ func TestRunStatus_CheckpointPushDisabledSettingsPrecedence(t *testing.T) { testutil.WriteFile(t, ".", ".entire/settings.local.json", tc.local) } testutil.AddRemote(t, ".", originRemoteName, "https://github.com/org/repo.git") + installStatusTestGitHooks(t) head := checkpointSyncTestCommit(t, "a.txt", "one") testutil.GitUpdateRef(t, ".", "refs/heads/"+paths.MetadataBranchName, head) for _, jsonOutput := range []bool{false, true} { @@ -2854,6 +2865,17 @@ func TestRunStatus_CheckpointPushDisabledAbsentWithoutEnabledEntire(t *testing.T // checkpointSyncTestCommit creates a commit in the cwd test repo and returns // its hash. setupTestRepo leaves the repo without commits, and both the v1 // counter and ref updates need at least one. +// installStatusTestGitHooks installs Entire's Git hooks in the current test +// repo. The unpushed counter promises the next push only while a pre-push hook +// will run it, so a fixture asserting that promise needs the hooks. +func installStatusTestGitHooks(t *testing.T) { + t.Helper() + if _, err := strategy.ReinstallGitHooks(t.Context()); err != nil { + t.Fatalf("ReinstallGitHooks: %v", err) + } + strategy.ClearHooksDirCache() +} + func checkpointSyncTestCommit(t *testing.T, name, content string) string { t.Helper() testutil.WriteFile(t, ".", name, content) @@ -2986,6 +3008,7 @@ func TestRunStatus_CheckpointSyncCounter_GitBranchAhead(t *testing.T) { setupTestRepo(t) writeSettings(t, testSettingsEnabled) testutil.AddRemote(t, ".", "origin", "https://example.com/origin.git") + installStatusTestGitHooks(t) checkpointSyncTestCommit(t, "a.txt", "one") second := checkpointSyncTestCommit(t, "b.txt", "two") // Local v1 with no origin-tracking ref: every v1 commit counts as unpushed @@ -3389,6 +3412,10 @@ func TestRunStatus_NativeHookDeliveryFailures(t *testing.T) { testutil.RunGit(t, dir, "remote", "add", "origin", "https://example.invalid/repo.git") tc.arrangeFailure(t, dir) strategy.ClearHooksDirCache() + // Unpushed checkpoints: the counter must not promise that a push + // no hook will run is going to sync them. + checkpointSyncTestCommit(t, "a.txt", "one") + testutil.GitUpdateRef(t, dir, "refs/heads/"+paths.MetadataBranchName, checkpointSyncTestCommit(t, "b.txt", "two")) var stdout bytes.Buffer if err := runStatus(t.Context(), &stdout, false, false); err != nil { @@ -3401,6 +3428,9 @@ func TestRunStatus_NativeHookDeliveryFailures(t *testing.T) { if !strings.Contains(out, "Checkpoints sync to: origin") { t.Errorf("status must retain the diagnostic destination, got:\n%s", out) } + if !strings.Contains(out, "2 checkpoints not yet on origin — they won't sync until Entire's Git hooks run again") { + t.Errorf("status must not promise the next push syncs checkpoints, got:\n%s", out) + } var jsonOut bytes.Buffer if err := runStatusJSON(t.Context(), &jsonOut); err != nil { From 8e401b243908997fd6a2b2556d02d9781b4f646c Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 23 Sep 2026 21:31:53 -0700 Subject: [PATCH 16/18] fix(hooks): warn when LEFTHOOK=0 skips Entire's Lefthook hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lefthook's launcher exits on LEFTHOOK=0 before reading any config, so once Entire runs as a Lefthook job it is skipped along with the user's own hooks. A commit made that way gets no checkpoint trailer and its turn is attributed to no commit; nothing inside Lefthook can opt out of it (its rc: file is sourced after the exit). CheckHookDelivery now reports that as SkippedBy, keeping OK true since nothing is broken. status warns beneath "Git hooks · via Lefthook" and drops the next-push promise from the unpushed counter, status --json carries hooks_skipped_by, and doctor adds a warning under its OK line. Co-Authored-By: Claude Opus 5.5 (1M context) Entire-Checkpoint: 01M38TYBGW4REG8Y69B2X662A8 --- cmd/entire/cli/doctor.go | 3 ++ cmd/entire/cli/status.go | 17 ++++++- cmd/entire/cli/status_test.go | 47 +++++++++++++++++++ cmd/entire/cli/strategy/lefthook.go | 17 ++++++- cmd/entire/cli/strategy/lefthook_test.go | 30 ++++++++++++ docs/development/checkpoint-implementation.md | 8 ++++ 6 files changed, 119 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 5ee83dd7a7..5bd43bc038 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -657,6 +657,9 @@ func checkGitHooks(cmd *cobra.Command, force bool) error { } } fmt.Fprintf(w, "✓ Git hooks: OK (via %s)\n", delivery.Manager) + if delivery.SkippedBy != "" { + fmt.Fprintf(w, " Warning: %s %s.\n", delivery.SkippedBy, hooksSkippedByEnvSuffix) + } return nil } fmt.Fprintf(w, "Git hooks: NOT DELIVERING (%s)\n", delivery.Manager) diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index 6627fcdf15..56a7c4037e 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -573,22 +573,31 @@ func countUnpushedCheckpointsForStatus(ctx context.Context, remoteName string) i // reach it, which is issue #2264. The destination line below is still shown — // it is true, and status is the only place it appears — but this line above it // removes the claim that delivery is working. It returns whether the hooks -// deliver, so the counter below can drop its promise about the next push. +// deliver in this environment — LEFTHOOK=0 skips them — so the counter below +// can drop its promise about the next push. func writeHookDeliveryLine(ctx context.Context, b *strings.Builder, sty statusStyles) bool { delivery := strategy.CheckHookDelivery(ctx) b.WriteString("\n") switch { case delivery.OK && delivery.Manager != "": b.WriteString(sty.render(sty.dim, " Git hooks · via "+delivery.Manager)) + if delivery.SkippedBy != "" { + b.WriteString("\n") + b.WriteString(sty.render(sty.yellow, " ! "+delivery.SkippedBy+" "+hooksSkippedByEnvSuffix)) + } case delivery.OK: b.WriteString(sty.render(sty.dim, " Git hooks · installed")) default: b.WriteString(sty.render(sty.yellow, " ! Checkpoints are NOT being captured: "+delivery.Reason)) b.WriteString(sty.render(sty.dim, " · run 'entire doctor'")) } - return delivery.OK + return delivery.OK && delivery.SkippedBy == "" } +// hooksSkippedByEnvSuffix completes the warning for HookDelivery.SkippedBy, +// shared by status and doctor so the two say the same thing. +const hooksSkippedByEnvSuffix = "is set: Lefthook skips Entire's hooks too, so commits made with it are not linked to checkpoints" + // writeCheckpointSyncLines reports the checkpoint sync destination (and the // unpushed counter, when non-zero) in the enabled status block, prefixed by the // disabled-pushing line when automatic pushing is off. No remotes configured @@ -1106,6 +1115,9 @@ type statusJSON struct { // HooksLefthookDeclined names a Lefthook local config Entire will not // write to, which is why Lefthook is not the one delivering. HooksLefthookDeclined string `json:"hooks_lefthook_declined,omitempty"` + // HooksSkippedBy names an environment setting (LEFTHOOK=0) under which the + // hook manager skips Entire's otherwise working hooks. + HooksSkippedBy string `json:"hooks_skipped_by,omitempty"` // CheckpointReadSourceUnknown reports that the read-source probe failed, // so no read source could be determined. Emitted only alongside // checkpoint_push_disabled, and checkpoint_sync_remote is then absent @@ -1225,6 +1237,7 @@ func runStatusJSON(ctx context.Context, w io.Writer) error { result.HooksManager = delivery.Manager result.HooksDeliverReason = delivery.Reason result.HooksLefthookDeclined = delivery.Declined + result.HooksSkippedBy = delivery.SkippedBy result.CheckpointReadFallback = syncInfo.ReadFallback result.CheckpointReadSourceUnknown = syncInfo.ReadSourceUnknown result.UnpushedCheckpoints = syncInfo.Unpushed diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 1d943421a7..fea9abbab9 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -3369,6 +3369,53 @@ func TestRunStatus_HookDeliveryLine(t *testing.T) { } } +// With LEFTHOOK=0 set, Lefthook skips Entire's registered hooks along with the +// user's, so status must say so — and must not promise that the next push +// syncs checkpoints, since that push would skip Entire's pre-push too. +func TestRunStatus_LefthookDisabledByEnv(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + setupTestRepo(t) + writeSettings(t, testSettingsEnabled) + testutil.AddRemote(t, ".", "origin", "https://example.com/origin.git") + if err := os.WriteFile("lefthook.yml", []byte("pre-commit: {}\n"), 0o644); err != nil { + t.Fatalf("write lefthook.yml: %v", err) + } + if _, err := strategy.EnsureLefthookIntegration(context.Background()); err != nil { + t.Fatalf("EnsureLefthookIntegration() error = %v", err) + } + installStatusTestGitHooks(t) + head := checkpointSyncTestCommit(t, "a.txt", "one") + testutil.GitUpdateRef(t, ".", "refs/heads/"+paths.MetadataBranchName, head) + t.Setenv("LEFTHOOK", "0") + + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + out := stdout.String() + if !strings.Contains(out, "Git hooks · via Lefthook") { + t.Errorf("registration is still correct and must be reported, got:\n%s", out) + } + if !strings.Contains(out, "LEFTHOOK=0 is set") { + t.Errorf("status must warn that LEFTHOOK=0 skips Entire's hooks, got:\n%s", out) + } + if !strings.Contains(out, "won't sync until Entire's Git hooks run again") { + t.Errorf("status must not promise the next push syncs checkpoints, got:\n%s", out) + } + + var jsonOut bytes.Buffer + if err := runStatusJSON(context.Background(), &jsonOut); err != nil { + t.Fatalf("runStatusJSON() error = %v", err) + } + var got statusJSON + if err := json.Unmarshal(jsonOut.Bytes(), &got); err != nil { + t.Fatalf("unmarshal status JSON: %v", err) + } + if !got.HooksDeliver || got.HooksManager != "Lefthook" || got.HooksSkippedBy != "LEFTHOOK=0" { + t.Errorf("hooks fields = %+v, want delivering via Lefthook, skipped by LEFTHOOK=0: %s", got, jsonOut.String()) + } +} + // #2264 is not Lefthook-specific: whatever makes the native hooks unreachable, // status must qualify its checkpoint destination with a capture warning. These // cases pin the distinct repo states from the issue's acceptance matrix. diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index 81485af172..a6ae3f34b9 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -816,6 +816,15 @@ func reconcileHookFiles(ctx context.Context) error { return nil } +// lefthookDisabledByEnv reports LEFTHOOK=0, the one value Lefthook's launcher +// exits on (`if [ "$LEFTHOOK" = "0" ]; then exit 0; fi`), or "". +func lefthookDisabledByEnv() string { + if os.Getenv("LEFTHOOK") == "0" { + return "LEFTHOOK=0" + } + return "" +} + func fileContains(root *os.Root, name, marker string) bool { data, err := osroot.ReadFileNoFollow(root, name) return err == nil && strings.Contains(string(data), marker) @@ -889,6 +898,12 @@ type HookDelivery struct { // OK is true, because it is the answer to "why is Entire not in Lefthook's // config" in a repository whose hooks are working fine without it. Declined string + // SkippedBy names an environment setting that makes the hook manager skip + // Entire's hooks in this environment even though they are wired up + // correctly — so OK stays true and the fix is not a repair. Today that is + // only LEFTHOOK=0: Lefthook's launcher exits on it before reading any + // config, so no registration can opt out of it. + SkippedBy string } // CheckHookDelivery reports whether Entire's hooks will fire in this @@ -917,7 +932,7 @@ func CheckHookDelivery(ctx context.Context) HookDelivery { Reason: fmt.Sprintf("Lefthook has no hook file for %s, so nothing runs Entire there.", strings.Join(uncovered, ", "))} } - return HookDelivery{OK: true, Manager: LefthookManagerName} + return HookDelivery{OK: true, Manager: LefthookManagerName, SkippedBy: lefthookDisabledByEnv()} } // A declined integration is permanent, not a repair pending: Entire's // own hooks are the arrangement in such a repo, so the answer is the diff --git a/cmd/entire/cli/strategy/lefthook_test.go b/cmd/entire/cli/strategy/lefthook_test.go index 8113ec93a0..f07ee91b9b 100644 --- a/cmd/entire/cli/strategy/lefthook_test.go +++ b/cmd/entire/cli/strategy/lefthook_test.go @@ -421,6 +421,36 @@ func TestCheckHookDelivery_Lefthook(t *testing.T) { require.Contains(t, got.Reason, "pre-push") } +// LEFTHOOK=0 makes Lefthook's launcher exit before it reads any config, so +// Entire's registration is skipped with everything else. The registration is +// still correct, so delivery stays OK; what changes is that this environment +// bypasses it, which is reported separately. +func TestCheckHookDelivery_LefthookDisabledByEnv(t *testing.T) { + dir := newLefthookRepo(t, "") + _, err := EnsureLefthookIntegration(t.Context()) + require.NoError(t, err) + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + for _, hook := range gitHookNames { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, hook), lefthookLauncher(hook), 0o755)) + } + ClearHooksDirCache() + + for _, tc := range []struct { + value, want string + }{ + {"0", "LEFTHOOK=0"}, + {"1", ""}, + {"", ""}, + } { + t.Setenv("LEFTHOOK", tc.value) + got := CheckHookDelivery(t.Context()) + require.True(t, got.OK, "LEFTHOOK=%q reason: %s", tc.value, got.Reason) + require.Equal(t, LefthookManagerName, got.Manager) + require.Equal(t, tc.want, got.SkippedBy, "LEFTHOOK=%q", tc.value) + } +} + // In a repo with no hook manager, delivery is the native hook state. func TestCheckHookDelivery_Native(t *testing.T) { dir := t.TempDir() diff --git a/docs/development/checkpoint-implementation.md b/docs/development/checkpoint-implementation.md index 103b38d9a3..0496f1d7d8 100644 --- a/docs/development/checkpoint-implementation.md +++ b/docs/development/checkpoint-implementation.md @@ -171,6 +171,14 @@ The manual-commit strategy (`manual_commit*.go`) does not modify the active bran installed natively — in a repo where nobody ran `lefthook install`, Entire's own hooks are the only delivery there is. This is also why `EnsureSetup` runs the Lefthook step *before* the native install. + **`LEFTHOOK=0` skips Entire too**, and nothing can prevent it while Entire + runs as a Lefthook job: Lefthook's launcher exits on it before reading any + config (its `rc:` file is sourced after that exit), and escaping it would + mean owning the hook files again, which is the fight above. A commit made + under it gets no checkpoint trailer and its turn is attributed to no commit; a push + made under it leaves checkpoints for the next push. `CheckHookDelivery` + reports it as `SkippedBy` — delivery stays OK because nothing is broken — + and `status` / `doctor` warn while it is set in their environment. Note: `checkpoint/configloader.go` overrides go-git's default config loader with a symlink-following `billy.Basic` (`osSymlinkFS`) — go-git's default reads config via `os.Root`, which rejects absolute symlinks in any path component (e.g. a `~/.config` managed by a dotfile tool), silently dropping global config so author identity fell back to "Unknown" and signing was skipped. From 9bb072252719ce4e2a895a8527f43e7ac7ea5650 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 24 Sep 2026 07:58:28 -0700 Subject: [PATCH 17/18] fix(hooks): stop warning that hk overwrites Entire's hooks on Git 2.54+ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hk 2.x installs Git's config-based hooks (hook..command) on Git 2.54+, which git runs alongside .git/hooks/* — verified with hk 2.1.0 on Git 2.55: both hk's hooks and Entire's fire, and re-running hk install leaves Entire's hook files alone. Only hk's legacy mode (older Git, or --legacy) writes shims over them. entire enable now warns about hk only when it will write shims: Git older than 2.54, an unknown version, or an hk shim already in the hooks directory. Otherwise it prints a note that no action is needed and names --legacy as the mode that does replace Entire's hooks. Co-Authored-By: Claude Opus 5.5 (1M context) Entire-Checkpoint: 01M39YSN6R5DK81ZPXH3AWKKYH --- cmd/entire/cli/strategy/hook_managers.go | 125 ++++++++++++++++-- cmd/entire/cli/strategy/hook_managers_test.go | 59 +++++++++ 2 files changed, 174 insertions(+), 10 deletions(-) diff --git a/cmd/entire/cli/strategy/hook_managers.go b/cmd/entire/cli/strategy/hook_managers.go index dd08bbfa44..be8d2db1db 100644 --- a/cmd/entire/cli/strategy/hook_managers.go +++ b/cmd/entire/cli/strategy/hook_managers.go @@ -5,9 +5,13 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" + "strconv" "strings" + "time" + "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" ) @@ -15,6 +19,9 @@ import ( type hookManager struct { Name string // "Husky", "Lefthook", "pre-commit", "Overcommit", "hk" ConfigPath string // relative path that triggered detection (e.g., ".husky/") + // ComposesWithEntire is set for hk when it installs Git's config-based + // hooks, which git runs alongside .git/hooks/* instead of replacing them. + ComposesWithEntire bool } // detectHookManagers checks the repository root for known hook manager config @@ -23,23 +30,26 @@ type hookManager struct { // Every manager listed here overwrites Entire's hooks when it installs them — // verified against pre-commit 4.6.2 ("Use -f to use only pre-commit", which // saves a .pre-commit.legacy copy), Overcommit 0.73.0 ("Moving old hooks"), and -// hk 1.58.1. What makes Lefthook different, and what EnsureLefthookIntegration +// hk 1.58.1 — hk only in its legacy mode, which writes .git/hooks/* shims: on +// Git 2.54+ hk 2.x installs config-based hooks (hook..command), which git +// runs alongside Entire's hook files (verified with hk 2.1.0 on Git 2.55). What +// makes Lefthook different, and what EnsureLefthookIntegration // exists for, is that it also reclaims .git/hooks/* at the top of every later // run, so reinstalling on the next agent turn never wins the race. func detectHookManagers(repoRoot string) []hookManager { var managers []hookManager checks := []hookManager{ - {"Husky", ".husky/"}, - {"pre-commit", ".pre-commit-config.yaml"}, - {"Overcommit", ".overcommit.yml"}, + {Name: "Husky", ConfigPath: ".husky/"}, + {Name: "pre-commit", ConfigPath: ".pre-commit-config.yaml"}, + {Name: "Overcommit", ConfigPath: ".overcommit.yml"}, } // Lefthook supports {.,}lefthook{,-local}.{yml,yaml,json,toml} for _, prefix := range []string{"", "."} { for _, variant := range []string{"", "-local"} { for _, ext := range []string{"yml", "yaml", "json", "toml"} { - checks = append(checks, hookManager{LefthookManagerName, prefix + "lefthook" + variant + "." + ext}) + checks = append(checks, hookManager{Name: LefthookManagerName, ConfigPath: prefix + "lefthook" + variant + "." + ext}) } } } @@ -47,7 +57,7 @@ func detectHookManagers(repoRoot string) []hookManager { // hk supports {.config/,}hk{,.local}.pkl for _, dir := range []string{"", ".config/"} { for _, variant := range []string{"", ".local"} { - checks = append(checks, hookManager{"hk", dir + "hk" + variant + ".pkl"}) + checks = append(checks, hookManager{Name: "hk", ConfigPath: dir + "hk" + variant + ".pkl"}) } } @@ -114,17 +124,107 @@ func hookManagerWarning(managers []hookManager, cmdPrefix, declined string) stri fmt.Fprintf(&b, " %s%s:\n", m.ConfigPath, spec.name) fmt.Fprintf(&b, " %s\n\n", cmdLine) } + case "hk": + if !m.ComposesWithEntire { + writeOverwritesAtInstallWarning(&b, m) + break + } + fmt.Fprintf(&b, "Note: %s detected (%s)\n\n", m.Name, m.ConfigPath) + fmt.Fprintf(&b, " %s uses Git's config-based hooks here (Git 2.54+), which run alongside\n", m.Name) + fmt.Fprintf(&b, " Entire's hooks instead of replacing them. No action needed.\n") + fmt.Fprintf(&b, " (`%s install --legacy` writes hook files instead, which do replace Entire's.)\n\n", m.Name) default: - fmt.Fprintf(&b, "Warning: %s detected (%s)\n\n", m.Name, m.ConfigPath) - fmt.Fprintf(&b, " %s overwrites Entire's hooks when it installs its own.\n", m.Name) - fmt.Fprintf(&b, " Entire reinstalls them on the next agent turn, but a commit made in\n") - fmt.Fprintf(&b, " between is not captured. Run 'entire enable' to restore them now.\n\n") + writeOverwritesAtInstallWarning(&b, m) } } return b.String() } +// writeOverwritesAtInstallWarning is the warning for a manager that replaces +// Entire's hook files when it installs its own but does not reclaim them later. +func writeOverwritesAtInstallWarning(b *strings.Builder, m hookManager) { + fmt.Fprintf(b, "Warning: %s detected (%s)\n\n", m.Name, m.ConfigPath) + fmt.Fprintf(b, " %s overwrites Entire's hooks when it installs its own.\n", m.Name) + fmt.Fprintf(b, " Entire reinstalls them on the next agent turn, but a commit made in\n") + fmt.Fprintf(b, " between is not captured. Run 'entire enable' to restore them now.\n\n") +} + +// hkComposesWithEntire reports whether hk's hooks run alongside Entire's here. +// hk 2.x uses Git's config-based hooks on Git 2.54+ and falls back to writing +// .git/hooks/* shims on older Git (or with --legacy); a shim already present +// settles it, since the next `hk install` rewrites those files. An unknown +// version is treated as the old behaviour, so the warning errs toward caution. +func hkComposesWithEntire(legacyShimInstalled bool, gitVersion string) bool { + if legacyShimInstalled { + return false + } + major, minor, ok := parseGitMajorMinor(gitVersion) + return ok && (major > 2 || (major == 2 && minor >= 54)) +} + +// parseGitMajorMinor reads the leading "major.minor" of a version such as +// "2.54.1.windows.1". +func parseGitMajorMinor(version string) (major, minor int, ok bool) { + parts := strings.SplitN(version, ".", 3) + if len(parts) < 2 { + return 0, 0, false + } + major, errMajor := strconv.Atoi(parts[0]) + minor, errMinor := strconv.Atoi(parts[1]) + if errMajor != nil || errMinor != nil { + return 0, 0, false + } + return major, minor, true +} + +// isHkShim recognises the hook file hk's legacy mode writes +// (`test "${HK:-1}" = "0" || exec hk run --from-hook "$@"`). +func isHkShim(content string) bool { + return strings.Contains(content, "hk run ") && strings.Contains(content, "--from-hook") +} + +// hkLegacyShimInstalled reports whether any file in the hooks directory is an +// hk shim. Best effort: an unreadable directory reads as no shim. +func hkLegacyShimInstalled(ctx context.Context) bool { + hooksDir, err := GetHooksDir(ctx) + if err != nil { + return false + } + root, err := hooksRootForRemoval(hooksDir) + if err != nil { + return false + } + entries, err := osroot.ReadDir(root, ".") + if err != nil { + return false + } + for _, e := range entries { + if !e.Type().IsRegular() { + continue + } + if data, err := osroot.ReadFileNoFollow(root, e.Name()); err == nil && isHkShim(string(data)) { + return true + } + } + return false +} + +// installedGitVersion returns `git --version`'s version token, or "". +func installedGitVersion(ctx context.Context) string { + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + out, err := exec.CommandContext(ctx, "git", "--version").Output() + if err != nil { + return "" + } + fields := strings.Fields(string(out)) + if len(fields) < 3 || fields[0] != "git" || fields[1] != "version" { + return "" + } + return fields[2] +} + // extractCommandLine returns the first non-shebang, non-comment, non-empty line // from a hook script. This is the actual command invocation line. func extractCommandLine(hookContent string) string { @@ -157,6 +257,11 @@ func CheckAndWarnHookManagers(ctx context.Context, w io.Writer, absolutePath boo // Best-effort: hook manager warnings are advisory, skip on resolution failure return } + for i := range managers { + if managers[i].Name == "hk" { + managers[i].ComposesWithEntire = hkComposesWithEntire(hkLegacyShimInstalled(ctx), installedGitVersion(ctx)) + } + } warning := hookManagerWarning(managers, cmdPrefix, declinedLefthookLocalConfig(ctx, repoRoot)) if warning != "" { fmt.Fprintln(w) diff --git a/cmd/entire/cli/strategy/hook_managers_test.go b/cmd/entire/cli/strategy/hook_managers_test.go index 5f53421639..7a714031fb 100644 --- a/cmd/entire/cli/strategy/hook_managers_test.go +++ b/cmd/entire/cli/strategy/hook_managers_test.go @@ -382,6 +382,65 @@ func TestHookManagerWarning_OverwritesAtInstall(t *testing.T) { } } +// On Git 2.54+ hk installs config-based hooks (hook..command), which git +// runs alongside .git/hooks/* — so hk does not touch Entire's hooks, and +// warning that it overwrites them is the false advice #2263 was about. +func TestHookManagerWarning_HkComposes(t *testing.T) { + t.Parallel() + warning := hookManagerWarning([]hookManager{{Name: "hk", ConfigPath: "hk.pkl", ComposesWithEntire: true}}, "entire", "") + if !strings.Contains(warning, "Note: hk detected (hk.pkl)") { + t.Errorf("composing hk should get a note, got %q", warning) + } + if strings.Contains(warning, "overwrites") || strings.Contains(warning, "Warning:") { + t.Errorf("composing hk must not be described as overwriting, got %q", warning) + } + if !strings.Contains(warning, "--legacy") { + t.Errorf("note should name the install mode that does overwrite, got %q", warning) + } +} + +func TestHkComposesWithEntire(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + legacyShim bool + gitVersion string + want bool + }{ + {"config hooks available", false, "2.54.0", true}, + {"newer git", false, "2.55.0", true}, + {"windows build suffix", false, "2.54.1.windows.1", true}, + {"major above 2", false, "3.0.0", true}, + {"too old for config hooks", false, "2.53.9", false}, + {"apple git", false, "2.50.1", false}, + {"unknown version", false, "", false}, + {"unparseable version", false, "garbage", false}, + // A shim in .git/hooks means hk is in legacy mode here, whatever git + // could do: the next `hk install` rewrites those files. + {"legacy shim installed", true, "2.55.0", false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := hkComposesWithEntire(tc.legacyShim, tc.gitVersion); got != tc.want { + t.Errorf("hkComposesWithEntire(%v, %q) = %v, want %v", tc.legacyShim, tc.gitVersion, got, tc.want) + } + }) + } +} + +func TestIsHkShim(t *testing.T) { + t.Parallel() + shim := "#!/bin/sh\ntest \"${HK:-1}\" = \"0\" || exec hk run pre-commit --from-hook \"$@\"\n" + if !isHkShim(shim) { + t.Error("hk's legacy shim must be recognised") + } + for _, other := range []string{"#!/bin/sh\n# Entire CLI hooks\nentire hooks git pre-push \"$1\"\n", "#!/bin/sh\nhk check\n"} { + if isHkShim(other) { + t.Errorf("not an hk shim: %q", other) + } + } +} + func TestHookManagerWarning_Empty(t *testing.T) { t.Parallel() From fc5d778c4d54a11bdf5f8cad885f09ab028bda3a Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 19 Oct 2026 14:49:11 -0700 Subject: [PATCH 18/18] fix(uninstall): name the Lefthook registration in the confirmation In a Lefthook repo .git/hooks/* are Lefthook's launchers, so the uninstall summary listed no git hooks and a repo with only the Lefthook registration read as not installed, leaving the extends entry behind. Check Entire's Lefthook artifacts on their own and show them as a row. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M5B27NP9BV35XK55A3XXWDS6 --- cmd/entire/cli/setup.go | 33 ++++++++++++++++++----------- cmd/entire/cli/setup_test.go | 33 +++++++++++++++++++++++++++++ cmd/entire/cli/strategy/lefthook.go | 27 +++++++++++++++++++++++ 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/cmd/entire/cli/setup.go b/cmd/entire/cli/setup.go index dbcbc5a63f..8e834eceb0 100644 --- a/cmd/entire/cli/setup.go +++ b/cmd/entire/cli/setup.go @@ -2871,6 +2871,9 @@ func runUninstall(ctx context.Context, w, errW io.Writer, force bool) error { // version is stale but still ours, and uninstall must still offer to remove // it rather than reporting that Entire is not installed here. gitHooksInstalled := strategy.AnyGitHookInstalled(ctx) + // Checked on its own: in a Lefthook repo .git/hooks/* are Lefthook's, so + // the check above misses the config files uninstall will still edit. + lefthookRegistered := strategy.LefthookIntegrationPresent(ctx) // One sweep, threaded onwards: each external plugin costs a subprocess to ask, // and the removal below must act on exactly what the summary showed. agHookState := getAgentHookState(ctx) @@ -2888,7 +2891,7 @@ func runUninstall(ctx context.Context, w, errW io.Writer, force bool) error { // reports it with its remedy, and the run exits non-zero rather than // asserting an absence it could not verify. The retired Gemini hooks get // the same treatment when their config could not be read. - if !entireDirExists && !gitHooksInstalled && sessionStateCount == 0 && + if !entireDirExists && !gitHooksInstalled && !lefthookRegistered && sessionStateCount == 0 && shadowBranchCount == 0 && len(agHookState.installed) == 0 && len(agHookState.unchecked) == 0 && !geminiHooks && geminiHooksErr == nil { fmt.Fprintln(w, "Entire is not installed in this repository.") @@ -2898,13 +2901,14 @@ func runUninstall(ctx context.Context, w, errW io.Writer, force bool) error { // Show confirmation prompt unless --force if !force { confirmed, err := confirmUninstall(p, uninstallSummary{ - entireDirExists: entireDirExists, - gitHooksInstalled: gitHooksInstalled, - sessionStateCount: sessionStateCount, - shadowBranchCount: shadowBranchCount, - hookState: agHookState, - geminiHooks: geminiHooks, - geminiHooksErr: geminiHooksErr, + entireDirExists: entireDirExists, + gitHooksInstalled: gitHooksInstalled, + lefthookRegistered: lefthookRegistered, + sessionStateCount: sessionStateCount, + shadowBranchCount: shadowBranchCount, + hookState: agHookState, + geminiHooks: geminiHooks, + geminiHooksErr: geminiHooksErr, }) if err != nil { return err @@ -3048,9 +3052,11 @@ func uninstallShadowBranches(ctx context.Context, p *uninstallPrinter) bool { type uninstallSummary struct { entireDirExists bool gitHooksInstalled bool - sessionStateCount int - shadowBranchCount int - hookState agentHookState + // lefthookRegistered: Entire's Lefthook config, scripts or extends entry. + lefthookRegistered bool + sessionStateCount int + shadowBranchCount int + hookState agentHookState // geminiHooks reports Entire hooks left in .gemini/settings.json by removed // Gemini CLI support; geminiHooksErr is set when that file could not be // checked. @@ -3065,7 +3071,7 @@ func confirmUninstall(p *uninstallPrinter, summary uninstallSummary) (bool, erro p.blank() p.plain("This will completely remove Entire from this repository:") p.blank() - rows := make([]explainRow, 0, 7) + rows := make([]explainRow, 0, 8) if len(summary.hookState.installed) > 0 { rows = append(rows, explainRow{Label: "agent hooks", Value: strings.Join(agentDisplayNames(summary.hookState.installed), ", ")}) } @@ -3083,6 +3089,9 @@ func confirmUninstall(p *uninstallPrinter, summary uninstallSummary) (bool, erro if summary.gitHooksInstalled { rows = append(rows, explainRow{Label: "git hooks", Value: "prepare-commit-msg, commit-msg, post-commit, post-rewrite, pre-push"}) } + if summary.lefthookRegistered { + rows = append(rows, explainRow{Label: "Lefthook", Value: "entire-lefthook.yml, .lefthook-local/, and Entire's extends entry in the Lefthook local config"}) + } if summary.sessionStateCount > 0 { rows = append(rows, explainRow{Label: "session states", Value: strconv.Itoa(summary.sessionStateCount)}) } diff --git a/cmd/entire/cli/setup_test.go b/cmd/entire/cli/setup_test.go index 3c422ae7c0..62605ae506 100644 --- a/cmd/entire/cli/setup_test.go +++ b/cmd/entire/cli/setup_test.go @@ -30,6 +30,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/strategy" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/spf13/cobra" + "github.com/stretchr/testify/require" ) // Note: Tests for hook manipulation functions (addHookToMatcher, hookCommandExists, etc.) @@ -1311,6 +1312,38 @@ func TestRunUninstall_Force_RemovesEntireDirectory(t *testing.T) { } } +// In a Lefthook repo .git/hooks/* are Lefthook's launchers, not Entire's +// hooks, yet uninstall edits the user's Lefthook local config. The summary +// must say so before asking. +func TestRunUninstall_SummaryNamesLefthookRegistration(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + dir := setupTestRepo(t) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook.yml"), []byte("pre-commit: {}\n"), 0o644)) + _, err := strategy.EnsureLefthookIntegration(context.Background()) + require.NoError(t, err) + require.False(t, strategy.AnyGitHookInstalled(context.Background()), "fixture: no native Entire hook") + + // force=false prints the summary, then stops at the no-terminal guard. + var stdout, stderr bytes.Buffer + require.Error(t, runUninstall(context.Background(), &stdout, &stderr, false)) + require.Contains(t, stdout.String(), "Lefthook") + require.Contains(t, stdout.String(), "entire-lefthook.yml") +} + +// A Lefthook registration on its own is still an install to remove. +func TestRunUninstall_Force_RemovesLefthookRegistrationAlone(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + dir := setupTestRepo(t) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lefthook.yml"), []byte("pre-commit: {}\n"), 0o644)) + _, err := strategy.EnsureLefthookIntegration(context.Background()) + require.NoError(t, err) + + var stdout, stderr bytes.Buffer + require.NoError(t, runUninstall(context.Background(), &stdout, &stderr, true)) + require.NotContains(t, stdout.String(), "not installed") + require.False(t, strategy.LefthookIntegrationPresent(context.Background())) +} + func TestRunUninstall_Force_RemovesGitHooks(t *testing.T) { setupTestRepo(t) diff --git a/cmd/entire/cli/strategy/lefthook.go b/cmd/entire/cli/strategy/lefthook.go index a6ae3f34b9..a7a83ae986 100644 --- a/cmd/entire/cli/strategy/lefthook.go +++ b/cmd/entire/cli/strategy/lefthook.go @@ -403,6 +403,33 @@ func LefthookIntegrationCurrent(ctx context.Context) (bool, error) { return artifactsExcluded(ctx, repoRoot, localConfigName), nil } +// LefthookIntegrationPresent reports whether any artifact +// RemoveLefthookIntegration would take is there: the extends entry, Entire's +// config, or one of its scripts. Unlike LefthookIntegrationCurrent, a partial +// or stale install counts. +func LefthookIntegrationPresent(ctx context.Context) bool { + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return false + } + root, err := worktreedir.OpenAt(repoRoot) + if err != nil { + return false + } + if extendsEntryPresent(root) { + return true + } + if owned, err := fileIsOwned(root, entireLefthookConfig); err == nil && owned { + return true + } + for _, hook := range gitHookNames { + if owned, err := fileIsOwned(root, lefthookScriptPath(hook)); err == nil && owned { + return true + } + } + return false +} + // RemoveLefthookIntegration removes Entire's artifacts, returning the number // removed. Files without Entire's marker are left alone. func RemoveLefthookIntegration(ctx context.Context) (int, error) {